Add read timeout to socket_listener #2571

soldierkam · 2017-03-24T23:17:18Z

Add TCP timeout support to socket_listener

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)
README.md updated (if adding a new plugin)

danielnelson

To be consistent with the rest of telegraf, we should use an internal.Duration for the duration.

Perhaps we should have a default though to avoid half open sockets. What do you think @phemmer ?

danielnelson · 2017-03-29T01:36:39Z

plugins/inputs/socket_listener/README.md

+  ## Read deadline (timeout).
+  ## Only applies to stream sockets (e.g. TCP).
+  ## 0 (default) is unlimited.
+  # read_deadline = 30


I think this should be timeout, the reason is that it is a relative time where a deadline would be absolute and I think the "read" part is implied because of the input type.

Well maybe read_timeout is better, naming is so hard :)

phemmer · 2017-03-31T21:07:14Z

Perhaps we should have a default though to avoid half open sockets. What do you think @phemmer ?

Hrm, hard question. I think I lean more towards no default. If the socket being left open and idle is a problem, then I would say it's up to the sender to shut it down, not the recipient. In fact the recipient shutting it down can cause data loss if it shuts it down while a message is in transit. So yeah, definitely no default.
However this does raise the issue of dead connection detection. We probably should add a feature for controlling TCP keep alives.

Oh, and we should never have a half-open socket. Once the sender closes their side, telegraf shuts down its side.

phemmer · 2017-03-31T21:13:23Z

~~Oh, why TCP only? Read timeouts (SetReadDeadline) is valid for datagram (UDP) sockets too.~~

Nevermind, yes it's valid, but different use case. For datagram we only have a single socket. It's not TCP where we have a separate socket for every connection. We don't want to shut down the whole socket if it's idle.

danielnelson · 2017-03-31T21:23:18Z

What distinguishes a half-open socket from a dead connection?

phemmer · 2017-03-31T21:28:15Z

What distinguishes a half-open socket from a dead connection?

Half-open is when one side has issued a Close() on the socket but the other side has not. It's perfectly acceptable (as far as the TCP protocol goes) for the reading side of a socket to be closed, but still send data on the writing side.

Dead connection can happen on a full-open, or half-open. Without TCP keep alives, the recipient will never notice if a connection is severed uncleanly (no FIN/RST packet sent. E.G. sender's power cord pulled). With the TCP keep alive, one side sends an empty packet, and the other side responds with an ACK. No response means dead connection.

danielnelson

Go ahead an update the CHANGELOG.md file as well.

danielnelson · 2017-04-06T20:54:53Z

plugins/inputs/socket_listener/socket_listener.go

@@ -112,6 +117,7 @@ type SocketListener struct {
 	ServiceAddress string
 	MaxConnections int
 	ReadBufferSize int
+	ReadTimeout    int


Sorry, I should have been more clear, this is the value that should be an internal.Duration. This will allow people to specify the timeout like "30s", so make sure to update the README as well.

No, that's me. Sorry for that.

-Rename timeout parameter -Use internal.Duration

danielnelson

I think we need to SetReadDeadline before the first read.

danielnelson · 2017-04-06T23:32:02Z

plugins/inputs/socket_listener/socket_listener.go

@@ -78,6 +80,9 @@ func (ssl *streamSocketListener) read(c net.Conn) {
 		for _, m := range metrics {
 			ssl.AddFields(m.Name(), m.Fields(), m.Tags(), m.Time())
 		}
+		if ssl.ReadTimeout.Duration > 0 {
+			c.SetReadDeadline(time.Now().Add(ssl.ReadTimeout.Duration * time.Second))


Now that this is a duration we don't want to multiple it

phemmer · 2017-04-07T01:35:41Z

I think we need to SetReadDeadline before the first read.

Correct. I'd recommend restructuring the loop a bit. Meaning don't set the deadline before the loop, and then again at the end of it. Just do something like:

for {
  if ssl.ReadTimeout.Duration > 0 { set deadline here }
  if !scnr.Scan() { break }
  ...
}

danielnelson · 2017-04-07T23:19:02Z

When testing I receive:

2017-04-07T23:16:32Z E! Error in plugin [inputs.socket_listener]: read tcp [::1]:8094->[::1]:60636: i/o timeout

I'm just connecting with nc localhost 8094 and sending nothing.

soldierkam · 2017-04-08T06:20:14Z

What "read_timeout" value you've got in config file and how long it take to receive this message?
I tested this on Linux 4.4.1 and it works ok. Im using telnet for testing.
I receive this message only if I set read_timeout to some value and I do not send anything for that amount of time. It would be better if this will be warning message.

danielnelson · 2017-04-10T23:54:39Z

Yeah this is on the actual timeout. I think we shouldn't log anything on timeout, or it should be debug level.

soldierkam · 2017-06-08T20:55:07Z

How can I log it at debug level? Accumulator have only AddError method. I see three options:

add AddDebug method to Accumulator
add Debug flag to plugin conf and use it like in https://github.com/influxdata/telegraf/blob/master/plugins/inputs/mailchimp/chimp_api.go#L150
just log message without if statement like in https://github.com/influxdata/telegraf/blob/master/internal/models/makemetric.go#L125

danielnelson · 2017-06-08T21:55:42Z

Go with #3 for now.

danielnelson · 2017-07-18T01:55:14Z

Merged as f5a8415

Add read timeout to socket_listener

6707cd4

danielnelson suggested changes Mar 29, 2017

View reviewed changes

nhaugo added this to the 1.3.0 milestone Mar 31, 2017

danielnelson suggested changes Apr 6, 2017

View reviewed changes

Read timeout cleanup

aa801fd

-Rename timeout parameter -Use internal.Duration

soldierkam force-pushed the readDeadline branch from f8cb46d to aa801fd Compare April 6, 2017 21:51

Merge branch 'master' into readDeadline

a77cf69

danielnelson suggested changes Apr 6, 2017

View reviewed changes

Setup read timeout at loop start

d51dfab

danielnelson modified the milestones: 1.3.0, 1.4.0 Apr 27, 2017

soldierkam added 5 commits June 14, 2017 22:02

Log timeout error on debug level

f7d16d7

Merge remote-tracking branch 'origin/master' into readDeadline

d7652b2

Format source code and amend plugin description

c08f77a

Update CHANGELOG

49b9e95

Fix nil pointer dereference

aa1bb6d

danielnelson approved these changes Jul 18, 2017

View reviewed changes

danielnelson closed this Jul 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add read timeout to socket_listener #2571

Add read timeout to socket_listener #2571

soldierkam commented Mar 24, 2017 •

edited

Loading

danielnelson left a comment

danielnelson Mar 29, 2017

danielnelson Mar 31, 2017

phemmer commented Mar 31, 2017 •

edited

Loading

phemmer commented Mar 31, 2017 •

edited

Loading

danielnelson commented Mar 31, 2017

phemmer commented Mar 31, 2017

danielnelson left a comment

danielnelson Apr 6, 2017

soldierkam Apr 6, 2017

danielnelson left a comment

danielnelson Apr 6, 2017

phemmer commented Apr 7, 2017

danielnelson commented Apr 7, 2017

soldierkam commented Apr 8, 2017

danielnelson commented Apr 10, 2017

soldierkam commented Jun 8, 2017

danielnelson commented Jun 8, 2017

danielnelson commented Jul 18, 2017

Add read timeout to socket_listener #2571

Add read timeout to socket_listener #2571

Conversation

soldierkam commented Mar 24, 2017 • edited Loading

Required for all PRs:

danielnelson left a comment

Choose a reason for hiding this comment

danielnelson Mar 29, 2017

Choose a reason for hiding this comment

danielnelson Mar 31, 2017

Choose a reason for hiding this comment

phemmer commented Mar 31, 2017 • edited Loading

phemmer commented Mar 31, 2017 • edited Loading

danielnelson commented Mar 31, 2017

phemmer commented Mar 31, 2017

danielnelson left a comment

Choose a reason for hiding this comment

danielnelson Apr 6, 2017

Choose a reason for hiding this comment

soldierkam Apr 6, 2017

Choose a reason for hiding this comment

danielnelson left a comment

Choose a reason for hiding this comment

danielnelson Apr 6, 2017

Choose a reason for hiding this comment

phemmer commented Apr 7, 2017

danielnelson commented Apr 7, 2017

soldierkam commented Apr 8, 2017

danielnelson commented Apr 10, 2017

soldierkam commented Jun 8, 2017

danielnelson commented Jun 8, 2017

danielnelson commented Jul 18, 2017

soldierkam commented Mar 24, 2017 •

edited

Loading

phemmer commented Mar 31, 2017 •

edited

Loading

phemmer commented Mar 31, 2017 •

edited

Loading