New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add network timeout support #26

Merged
merged 25 commits into from Jan 8, 2019

Conversation

Projects
None yet
4 participants
@tomponline
Copy link
Contributor

tomponline commented Jun 29, 2017

Updates beanstalk client to have a default connection timeout of 10s, and to set 10s TCP keepalive messages on the connection so that connection hangs can be detected.

Defaults to 10s for both types of timeouts with no application changes.

Addressing #23

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Jul 4, 2017

@kr I would appreciate any feedback you have on this, and perhaps it could be merged if you are happy with it. Thanks!

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Nov 2, 2017

@kr any update on this? Are you happy to merge? thanks

@JensRantil
Copy link

JensRantil left a comment

Nice! Some general feedback:

  • What do you think of removing the Dial(...) function altogether from the library instead of implementing dial timeout here? There are other ways of creating a new.Conn than through net.Dial(...) and net.DialTimeout(...) and we can't possibly mirror all of those creation cases in this library. I understand that removing the functions will not be a backwards compatible change, but I'm willing to do that to simplify this library.
  • #23 discusses writeTimeout. Any reason you left that out?
conn.go Outdated
}

// DialTimeout connects to the given address on the given network using
// net.DialTimeout with a supplied timeout

This comment has been minimized.

@JensRantil

JensRantil Aug 19, 2018

Weird wrapping.

This comment has been minimized.

@tomponline

tomponline Aug 19, 2018

Contributor

Will fix.

conn.go Outdated
@@ -120,10 +153,18 @@ func (c *Conn) printLine(cmd string, args ...interface{}) {
}

func (c *Conn) readResp(r req, readBody bool, f string, a ...interface{}) (body []byte, err error) {
readTimeout := c.readTimeout
//For reserve-with-timeout commands, add reserve time to read timeout.

This comment has been minimized.

@JensRantil

JensRantil Aug 19, 2018

For consistency, please start comments with // , not //.

conn.go Outdated
@@ -9,18 +9,36 @@ import (
"time"
)

//DefaultConnTimeout time in seconds to wait for connection to beanstalk server.

This comment has been minimized.

@JensRantil

JensRantil Aug 19, 2018

For consistency, please start comments with // , not //.

This comment has been minimized.

@tomponline

tomponline Aug 19, 2018

Contributor

@JensRantil do you mean a space after the "//", I can't quite tell the difference from your post.

This comment has been minimized.

@JensRantil

JensRantil Aug 19, 2018

Oh, yes. Looks like Markdown scrubbed my space...

conn.go Outdated
@@ -31,26 +49,41 @@ var (
)

// NewConn returns a new Conn using conn for I/O.
func NewConn(conn io.ReadWriteCloser) *Conn {
func NewConn(conn ReadWriteCloserTimeout) *Conn {

This comment has been minimized.

@JensRantil

JensRantil Aug 19, 2018

Any reason why not using a net.Conn here instead?

This comment has been minimized.

@tomponline

tomponline Aug 19, 2018

Contributor

@JensRantil my thinking behind this was that to try and minimise the size of the interface required here, and only increase it by the size needed.

However if we expand this to support general deadline, writedeadline then it would be closer to https://golang.org/pkg/net/#Conn although arguable other functions in that interface aren't needed currently (like the local and remote addresses).

This comment has been minimized.

@JensRantil

JensRantil Aug 19, 2018

Ah, makes sense. Agree with you using an interface of our own makes sense (since the local and remote addresses aren't needed). 👌

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Aug 19, 2018

@JensRantil I added DialTimeout to mirror the Go stdlib's net conn package's DialTimeout function:

https://golang.org/pkg/net/#DialTimeout

From what I can see, a net.Conn only has Dial and DialTimeout.

Personally, I'm not really a fan of the 'defaults' being a connection with no timeouts, as it can cause big problems in the real-world of networking. This is why I added the default 10s timeout when using Dial().

I'm not sure what you mean about removing both Dial and DialTimeout, as it feels like it would make it harder for newcomers to get started with the package (as they would have to construct a net.Conn themselves first).

That being said, if we made DialTimeout unexported, or just change Dial to use the default timeouts, then we could merge this PR to make the package 'real world network' safe, without changing the exported API.

Then we could discuss the options for removing the Dial functions in a separate issue perhaps?

@JensRantil

This comment has been minimized.

Copy link

JensRantil commented Aug 19, 2018

From what I can see, a net.Conn only has Dial and DialTimeout.

No, there's also https://golang.org/pkg/net/#FileConn and https://golang.org/pkg/net/#Pipe. Also, since net.Conn is an interface there are likely lots of other implementations in the world :)

Personally, I'm not really a fan of the 'defaults' being a connection with no timeouts, as it can cause big problems in the real-world of networking.

I agree, but I'm not sure it's something we should work around downstream..

I'm not sure what you mean about removing both Dial and DialTimeout, as it feels like it would make it harder for newcomers to get started with the package (as they would have to construct a net.Conn themselves first).

Yeah, I guess it's a trade-off between library scoping, future maintenance vs. usability. Since I just became the maintainer here I'm going through stuff trying to keep the scope as tight as possible. I'm a big fan of small sharp tools that does very little but very well.

That being said, if we made DialTimeout unexported, or just change Dial to use the default timeouts, then we could merge this PR to make the package 'real world network' safe, without changing the exported API.

Then we could discuss the options for removing the Dial functions in a separate issue perhaps?

Sounds like an excellent plan! 👌 You think you could make that change?

Also, thanks a lot for fixing this!

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Aug 20, 2018

@JensRantil sounds good, yes will push up an updated PR soon.

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Aug 23, 2018

hi @JensRantil ive made those modifications.

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Aug 28, 2018

Hi @JensRantil is this OK to merge now/ I'm preparing a new release of https://github.com/tomponline/beanstalkworker/ and would like to include switching over to go-beanstalk as part of it. Thanks

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Sep 3, 2018

Hi @JensRantil any chance this could be merged, it'd be great to be able to get rid of my existing fork. Cheers

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Sep 17, 2018

@JensRantil hello? Can this be merged please?

@dfinkel
Copy link

dfinkel left a comment

Disclaimer: I'm not a maintainer of this package.

I have some problems with the way this adds timeouts. (see my code-comments)

conn.go Outdated
line, err := c.c.ReadLine()
for strings.HasPrefix(line, "WATCHING ") || strings.HasPrefix(line, "USING ") {
c.netConn.SetReadDeadline(time.Now().Add(readTimeout))

This comment has been minimized.

@dfinkel

dfinkel Sep 19, 2018

I strongly suspect setting deadlines like this is going to lead to the connection getting out of sync, leading to things like:

  • Portions of the response for one command being interpreted as the first line of the response for a subsequent one.
  • The response for one command being interpreted as the response for the next one because the original one timed out.

Fundamentally, I think the wire protocol for beanstalkd uses too much implicit state to safely support such deadlines.

As an example:
Let's say there's a network blip so, the response to a put gets delayed (or equivalently from the client's side, the request is delayed on its way to beanstalkd).

In that case, as this is written, the initial request will timeout, and the application gets back:
ConnError{c, "put", sometimeoutErr} (and job ID 0)

In the mean time, the response makes it to our client, with, say ID 523.

However, most applications don't put just one job, so the next put now comes along and the client now gets to this line again; this time, there's a response waiting for it.
If the tube is the same, the client skips the use line and goes directly to a put.

  • This time, it doesn't matter how fast that request is, the client will use the response that's queued from the previous request, rather than the one directly associated with the response.

This comment has been minimized.

@tomponline

tomponline Sep 20, 2018

Contributor

Hi @dfinkel thanks for your feedback.

As I understand it, the Go conn package requires that the read timeout be set before each read, this is why it is in the form of an absolute deadline in time, rather than a duration.

If reading the response times out, then as I understand it the connection itself is torn down (this is certainly my intention and experience of the behaviour), so that no other commands can be sent or read from the connection, and a new one must be established.

So I can appreciate what you're saying here, but I don't think it will actually happen that way, as the read will return with a ConnError

https://github.com/beanstalkd/go-beanstalk/pull/26/files#diff-a2ddca1600f34abb91244424f8431c2cL130

The alternative is that a network glitch causes the TCP connection to hang and your process will wait indefinitely for a response (we've experienced this in real life, which is what prompted me to add this PR).

This comment has been minimized.

@dfinkel

dfinkel Sep 20, 2018

You are correct that the go net.Conn interface requires that the timeout be set to an absolute value. However, the connection is definitely not torn down if it fails.

The only effects are: all subsequent reads (or writes if that deadline is set) will fail with an i/o timeout error until the deadline is updated again.

The documentation on net.Conn.SetTimeout() documentation alludes to this when it mentions that this can be used for idle polling.

The read will return a ConnError, but that's not always something that should be considered fatal.

e.g. The beanstalk.ConnError type wraps ErrTimeout and ErrDeadline which are non-fatal and just reflect either: 1) there is another job whose ttr is about to expire, or 2) the timeout on a reserve-timeout call (any Reserve() call in this client) has expired. Neither should trigger a connection tear-down.

I appreciate that some lost packets can cause the client to hang indefinitely as-is. In a request handling context where one is only using Put to push jobs, it even makes sense to have deadlines and kill the connection on most errors. However, this is a generic library that handles both producers and consumers. The beanstalk protocol is fundamentally not designed for this, and as-is the client will end up in an undefined state after a timeout, such that it will return garbage results to the client.

If you intend for it to close the connection on timeout, you'll have to add a c.Close and document it.

With that said, I think a better first step would be to constrain the implementation to using *net.TCPConn (or at least an interface that supports its SetKeepAlive() and SetKeepAlivePeriod()).
This way if the connection gets closed on the other side during a network blip, the connection can actually get a reset packet once the host is reachable, which will kill the connection, causing it to return io.EOF on any hanging request.

Granted, this is incomplete protection against network blackholes, but it does avoid adding a new poisoned state to the connection.

conn.go Outdated
const DefaultConnTimeout = 10

// DefaultReadTimeout time in seconds to wait for response from beanstalk server.
const DefaultReadTimeout = 10

This comment has been minimized.

@dfinkel

dfinkel Sep 19, 2018

Please don't put completely arbitrary deadlines in libraries by default.

10 seconds may seem reasonable most of the time, but if I have a batch process failing operations because for some reason the request takes 10.5s, I'm going to be very angry at someone if that gets me paged.

A connection timeout is reasonable to have, but I'm not a fan of default deadlines because they turn performance problems to debug at my leisure into hard-failures that wake someone up at night (or give them an error-page).

This comment has been minimized.

@tomponline

tomponline Sep 20, 2018

Contributor

I originally provided the ability to set a custom timeout but @JensRantil wasn't keen on changing the interface, so I made the timeouts (what I thought) was a sensible default.

Your comment about the batch/job times taking longer than the arbitrary time is valid, but I dont believe this will be an issue, as I have considered it.

This is because when a job has been received and is being processed by a consumer, there is no timeout in effect because the package is not 'reading' (or waiting for a response) from the beanstalk server - so the read timeout doesn't apply.

Where I had to be careful when implementing this is the reserve request, which can wait an arbitrary time specified by the client to wait for a job before giving up.

However I have handled this by setting the read timeout for that request to the wait time + 10s. Here https://github.com/beanstalkd/go-beanstalk/pull/26/files#diff-a2ddca1600f34abb91244424f8431c2cR174

Hope this allays your concerns.

This comment has been minimized.

@dfinkel

dfinkel Sep 20, 2018

Unfortunately, I still have concerns here.

The protocol, (and this client) support multiple job reservations per connection. If the connection must be torn down due to a timeout when attempting to reserve an nth job, that now releases all the jobs associated with this connection, when it might be alright for my batch job processor to wait a bit longer.

In user-facing request contexts, I think having deadlines is useful, but in batch-contexts it's much easier to debug something that's hung than crashed (or retrying with errors).

@dfinkel
Copy link

dfinkel left a comment

Sorry to be such a downer here.
My replies got a bit longer than I really intended. I guess I have a bit to say about deadlines.

conn.go Outdated
const DefaultConnTimeout = 10

// DefaultReadTimeout time in seconds to wait for response from beanstalk server.
const DefaultReadTimeout = 10

This comment has been minimized.

@dfinkel

dfinkel Sep 20, 2018

Unfortunately, I still have concerns here.

The protocol, (and this client) support multiple job reservations per connection. If the connection must be torn down due to a timeout when attempting to reserve an nth job, that now releases all the jobs associated with this connection, when it might be alright for my batch job processor to wait a bit longer.

In user-facing request contexts, I think having deadlines is useful, but in batch-contexts it's much easier to debug something that's hung than crashed (or retrying with errors).

conn.go Outdated
line, err := c.c.ReadLine()
for strings.HasPrefix(line, "WATCHING ") || strings.HasPrefix(line, "USING ") {
c.netConn.SetReadDeadline(time.Now().Add(readTimeout))

This comment has been minimized.

@dfinkel

dfinkel Sep 20, 2018

You are correct that the go net.Conn interface requires that the timeout be set to an absolute value. However, the connection is definitely not torn down if it fails.

The only effects are: all subsequent reads (or writes if that deadline is set) will fail with an i/o timeout error until the deadline is updated again.

The documentation on net.Conn.SetTimeout() documentation alludes to this when it mentions that this can be used for idle polling.

The read will return a ConnError, but that's not always something that should be considered fatal.

e.g. The beanstalk.ConnError type wraps ErrTimeout and ErrDeadline which are non-fatal and just reflect either: 1) there is another job whose ttr is about to expire, or 2) the timeout on a reserve-timeout call (any Reserve() call in this client) has expired. Neither should trigger a connection tear-down.

I appreciate that some lost packets can cause the client to hang indefinitely as-is. In a request handling context where one is only using Put to push jobs, it even makes sense to have deadlines and kill the connection on most errors. However, this is a generic library that handles both producers and consumers. The beanstalk protocol is fundamentally not designed for this, and as-is the client will end up in an undefined state after a timeout, such that it will return garbage results to the client.

If you intend for it to close the connection on timeout, you'll have to add a c.Close and document it.

With that said, I think a better first step would be to constrain the implementation to using *net.TCPConn (or at least an interface that supports its SetKeepAlive() and SetKeepAlivePeriod()).
This way if the connection gets closed on the other side during a network blip, the connection can actually get a reset packet once the host is reachable, which will kill the connection, causing it to return io.EOF on any hanging request.

Granted, this is incomplete protection against network blackholes, but it does avoid adding a new poisoned state to the connection.

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Sep 21, 2018

@dfinkel please don't apologise, your feedback on where this approach falls down in certain usage scenarios is valuable, and even more so is your suggestion for an alternative approach using TCP level keep-alives.

My aim with this patch was to make the go-beanstalk library "real world ready", and avoid others experiencing the production issues we've experienced with this library (programs hanging for days on the reserve command).

I've been using this patch in production for about a year now with no ill effects, however I've been using a restricted sub-set of features in a higher level consumer wrapper (https://github.com/tomponline/beanstalkworker) that disconnects the socket at the first sign of trouble.

I had not considered the multi-reserve scenario or the scenario where people would continue to use the socket after a failure had occurred.

I like the idea of using a TCP socket level keep alive to detect far end failures and cause the socket to close. This feels like a lite-touch way of achieving the aim of making this library recoverable in the scenario of a network glitch.

I'll make the changes and re-submit.

Thanks
Tom

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Sep 21, 2018

@dfinkel @JensRantil I've had a go at implementing TCP KeepAlives, and it seems to be working as desired.

I used a simple test program:

package main

import (
        "fmt"
	"github.com/tomponline/beanstalk"
        "time"
)


func main() {
	var conn, err = beanstalk.Dial("tcp", "127.0.0.1:11300")
       if err != nil {
                panic(err)
        }
	id, body, err := conn.Reserve(3600 * time.Second)
        if err != nil {
                fmt.Println(err)
        }

	id, body, err = conn.Reserve(3600 * time.Second)
        if err != nil {
                panic(err)
        }

	fmt.Println("job", id)
        fmt.Println(string(body))
}

Then checked that once the client had connected I could see keepalives being sent:

tcpdump -i lo -nn
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on lo, link-type EN10MB (Ethernet), capture size 262144 bytes
12:01:48.010387 IP 127.0.0.1.36218 > 127.0.0.1.11300: Flags [.], ack 2977344127, win 342, options [nop,nop,TS val 58779104 ecr 58769078], length 0
12:01:48.010469 IP 127.0.0.1.11300 > 127.0.0.1.36218: Flags [.], ack 1, win 342, options [nop,nop,TS val 58779104 ecr 58769078], length 0
12:01:58.026384 IP 127.0.0.1.36218 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 58789120 ecr 58779104], length 0
12:01:58.026468 IP 127.0.0.1.11300 > 127.0.0.1.36218: Flags [.], ack 1, win 342, options [nop,nop,TS val 58789120 ecr 58769078], length 0
12:02:08.042363 IP 127.0.0.1.36218 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 58799136 ecr 58789120], length 0
12:02:08.042443 IP 127.0.0.1.11300 > 127.0.0.1.36218: Flags [.], ack 1, win 342, options [nop,nop,TS val 58799136 ecr 58769078], length 0
^C

At that point, I used iptables to block the connection between the client and the server by just dropping packets:

iptables -A INPUT -p tcp --dport 11300 -j DROP

I then saw the client sending keepalives, but no response from server:

[root@el7build01 ~]# tcpdump -i lo -nn
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on lo, link-type EN10MB (Ethernet), capture size 262144 bytes
12:05:42.058367 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 4018313561, win 342, options [nop,nop,TS val 59013152 ecr 59003136], length 0
12:05:52.074399 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 59023168 ecr 59003136], length 0
12:06:02.090388 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 59033184 ecr 59003136], length 0
12:06:12.106377 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 59043200 ecr 59003136], length 0
12:06:22.122400 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 59053216 ecr 59003136], length 0
12:06:32.138381 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 59063232 ecr 59003136], length 0
12:06:42.154387 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 59073248 ecr 59003136], length 0
12:06:52.170384 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 59083264 ecr 59003136], length 0
12:07:02.186385 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [.], ack 1, win 342, options [nop,nop,TS val 59093280 ecr 59003136], length 0
12:07:12.202395 IP 127.0.0.1.36222 > 127.0.0.1.11300: Flags [R.], seq 1, ack 1, win 342, options [nop,nop,TS val 0 ecr 59003136], length 0
^C

After 10 attempts, the program failed with the following output:

go run beanstalk_demo.go 
reserve-with-timeout: read tcp 127.0.0.1:36222->127.0.0.1:11300: read: connection timed out
panic: reserve-with-timeout: write tcp 127.0.0.1:36222->127.0.0.1:11300: write: broken pipe

goroutine 1 [running]:
main.main()
	/root/go/beanstalk_demo.go:22 +0x28b
exit status 2

You can see that the 2nd call to reserve fails immediately after the first reserve fails.

So seems to be working, with a default keepalive period of 10s, it takes 100s to timeout and detect connection problem.

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Sep 21, 2018

Also @JensRantil @dfinkel it looks like someone else has had this idea back in 2017, and has submitted an a similar (albeit interfacing changing) PR. #27 Hopefully if this works and we can merge, we can close both issues.

@tomponline tomponline referenced this pull request Sep 21, 2018

Closed

support keep alive #27

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Oct 3, 2018

@JensRantil any thoughts on this? Please can it be merged yet?

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Jan 3, 2019

@kr would it be possible to get this PR merged, I think @JensRantil has been rather busy of late and is unable to commit time to this project. Thanks

@kr

This comment has been minimized.

Copy link
Member

kr commented Jan 6, 2019

It looks like 2 separate changes are here. One is updating the import paths for the new location of this module, and the other is adding the timeout. It would be good to have the import paths updated in a separate PR before this one.

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Jan 7, 2019

@kr no problem, will get that done shortly.

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Jan 7, 2019

@kr this should be good to merge now. Thanks

conn.go Outdated
@@ -9,6 +9,12 @@ import (
"time"
)

// DefaultConnTimeout time in seconds to wait for connection to beanstalk server.
const DefaultConnTimeout = 10

This comment has been minimized.

@kr

kr Jan 7, 2019

Member

IMO it would be a little better for this to be a time.Duration instead of an untyped number.

Also, I think it would be better to name it DefaultDialTimeout, since "dial timeout" is more specific than "conn timeout" and better conveys that it only applies to establishing the conn, and not for the rest of the conn's lifetime.

Something like:

Suggested change Beta
const DefaultConnTimeout = 10
// DefaultDialTimeout is the time to wait for a connection to the beanstalk server.
const DefaultDialTimeout = 10 * time.Second

This comment has been minimized.

@tomponline

tomponline Jan 7, 2019

Contributor

@kr sounds good will do.

conn.go Outdated
const DefaultConnTimeout = 10

// DefaultKeepAliveTimeout time in seconds to send TCP keepalive messages.
const DefaultKeepAliveTimeout = 10

This comment has been minimized.

@kr

kr Jan 7, 2019

Member

Same for this one.

And I'd suggest using "period" instead of "timeout" in this name, for consistency with package net.

Suggested change Beta
const DefaultKeepAliveTimeout = 10
// DefaultKeepAlivePeriod is the default period between TCP keepalive messages.
const DefaultKeepAlivePeriod = 10 * time.Second
conn.go Outdated
func Dial(network, addr string) (*Conn, error) {
c, err := net.Dial(network, addr)
connTimeout := time.Duration(DefaultConnTimeout) * time.Second
return DialTimeout(network, addr, connTimeout)

This comment has been minimized.

@kr

kr Jan 7, 2019

Member

And if you like the above suggestions, then this would of course be:

Suggested change Beta
return DialTimeout(network, addr, connTimeout)
return DialTimeout(network, addr, DefaultConnTimeout)
conn.go Outdated
if err != nil {
return nil, err
}
if tcpConn, ok := c.(*net.TCPConn); ok {
tcpConn.SetKeepAlive(true)
tcpConn.SetKeepAlivePeriod(DefaultKeepAliveTimeout * time.Second)

This comment has been minimized.

@kr

kr Jan 7, 2019

Member

And same here:

Suggested change Beta
tcpConn.SetKeepAlivePeriod(DefaultKeepAliveTimeout * time.Second)
tcpConn.SetKeepAlivePeriod(DefaultKeepAlivePeriod)
conn.go Outdated
// DialTimeout connects to the given address on the given network using net.DialTimeout
// with a supplied timeout and then returns a new Conn for the connection.
func DialTimeout(network, addr string, timeout time.Duration) (*Conn, error) {
c, err := net.DialTimeout(network, addr, timeout)

This comment has been minimized.

@kr

kr Jan 7, 2019

Member

It might be easier to use a net.Dialer to set the keepalive. Something like

Suggested change Beta
c, err := net.DialTimeout(network, addr, timeout)
dialer := &net.Dialer{
Timeout: timeout,
KeepAlive: DefaultKeepAlivePeriod,
}
c, err := dialer.Dial(network, addr)

And then the rest of this function would be less complex.

This comment has been minimized.

@tomponline

tomponline Jan 7, 2019

Contributor

@kr this looks great, will change to this, TIL about the Dialer interface's KeepAlive property :)

conn.go Outdated
@@ -41,13 +47,24 @@ func NewConn(conn io.ReadWriteCloser) *Conn {
return c
}

// Dial connects to the given address on the given network using net.Dial
// and then returns a new Conn for the connection.
// Dial connects to the given address on the given network using net.DialTimeout

This comment has been minimized.

@kr

kr Jan 7, 2019

Member
Suggested change Beta
// Dial connects to the given address on the given network using net.DialTimeout
// Dial connects to addr on the given network using DialTimeout
@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Jan 7, 2019

@kr I've made the changes you suggested, hows that? thanks

@kr kr merged commit 28608e9 into beanstalkd:master Jan 8, 2019

@kr

This comment has been minimized.

Copy link
Member

kr commented Jan 8, 2019

Thank you so much for all this work!

@tomponline

This comment has been minimized.

Copy link
Contributor

tomponline commented Jan 8, 2019

@kr no problem, thanks for merging.

This should allow this other PR to be closed now too: #27

And may also address this issue: #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment