add option to disable SIGPIPE on BSD-like systems #111
Conversation
|
Is it worth this much change vs the one liner suggested in that ticket. Timeouts gone etc |
|
That didn't work, I tried it. |
|
I'm not sure that guy tried it either. It doesn't sound like the first guy did either, he just stuck with his solution which is essentially what I did here |
|
Also apparently that one liner breaks things on Android, which is why his patch got reverted out of Go 1.8. |
|
K. Well as long as we can find a minimal change. Maybe @maxtaco should review |
|
Yeah I tagged him and Jeremy. |
|
Also this change is not that large, it just looks a little large because in order to be able to set the option on a TLS connection I just needed to change things a little in a mostly superficial way. |
|
Cool. With the keep alive back it seems close |
|
Yeah I put it back, but I don't think it actually does anything, AWS ELB doesn't count that as a connection actually being alive. Both Gregor and MD server have their own keep alive mechanisms to actually solve the problem, so I contemplated just removing it. But in the interest of making the minimal change, I think it is fine to keep it, it doesn't hurt anything. |
rpc/connection.go
Outdated
| @@ -75,6 +76,14 @@ func (t *connTransport) Dial(context.Context) (Transporter, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
|
|
|||
| // If the client has requested to disable SIGPIPE, then do so now | |||
There was a problem hiding this comment.
Very minor but I think all the other comments in this file end in punctuation, so would be nice to keep that up here and below.
| import "net" | ||
|
|
||
| func DisableSigPipe(c net.Conn) error { | ||
| return nil |
There was a problem hiding this comment.
Should this panic or return an "unsupported" error instead? We shouldn't just ignore an option the user passed in and let them believe we're honoring it, right?
There was a problem hiding this comment.
Sure, how about something like:
return errors.New("DisableSigPipe not supported")
There was a problem hiding this comment.
Although this makes it so that we need to detect whether this is supported on the clients of this library, which makes that code a lot more verbose. With this just doing nothing all that is simpler.
There was a problem hiding this comment.
How about I just add a better comment on DisableSigPipe that explains it is only available on Darwin?
There was a problem hiding this comment.
Just thought it was the more flexible option, since there might be other use cases of the library other than the client and KBFS at some point.
There was a problem hiding this comment.
If you really want an option for future-proofing, maybe you can invert it to something like EnableDarwinSigPipe, which would be false by default, in case we ever have a situation where the caller needs to ensure it's enabled?
There was a problem hiding this comment.
I can just nuke the option though, I think that is a good suggestion.
There was a problem hiding this comment.
I just put up a new commit that ditches the option, let me know what you think.
The point of this patch is to address the iOS app force closing when foregrounded after being in the background for awhile. See more info here:
golang/go#17393
We add an option to disable
SIGPIPEon Darwin builds on new connections using the transports provided by this library (note only the TLS transport really matters in production). Since Linux and Windows don't have this option, it is off for those builds.