Skip to content
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 keep alive to tcp socket #86

Closed
wants to merge 3 commits into from

Conversation

jagonalez
Copy link

@jagonalez jagonalez commented Jul 25, 2022

Description

This PR adds keep alive to the socket used to connect to a Faktory server. (see contribsys/faktory#316 - the author recommends using keep alive to protect against timeouts)

Unfortunately the socket library does not have a simple way of setting socket options in it's connectTo method. So instead we're using connectFromSocket and copying some of the code from socket's connectTo method to set the keep-alive.

Testing

To test this change run the examples here: https://github.com/freckle/faktory_worker_haskell#examples
or test a producer and worker used within your application

Copy link
Member

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to tag in @eborden to review please -- this is a bit above my networking pay-grade.

(socket (addrFamily addr) (addrSocketType addr) (addrProtocol addr))
close
(\sock -> do
setSocketOption sock KeepAlive 1
Copy link
Member

@pbrisbin pbrisbin Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we could try to upstream some kind of connectToWith to connection that takes a function to call at this point, then redefine connectTo as,

connectTo = connectToWith $ const $ pure ()

And we could use connectToWith $ \sock -> setSocketOption sock KeepAlive 1.

Copy link
Member

@pbrisbin pbrisbin Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, extending ConnectionParams to have some kind of "setup socket" field would probably be cleaner. Though, since that type is exported by the module, that change would be a major version bump (whereas what I described above would not).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbrisbin I agree. I would not like this code to live in this library. It would be much better placed upstream. However I see nothing wrong with enabling KeepAlive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an old issue open speaking about this

vincenthz/hs-connection#48

@vincenthz has been inactive in that repo since 2019, so it is possible connection is unmaintained.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, I was suggestion we still merge this work, but seek to remove it later by pushing upstream.

Given the inactivity though, maybe we just accept it has to live here?

(\sock -> do
setSocketOption sock KeepAlive 1
S.connect sock (addrAddress addr)
return (sock, addrAddress addr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a near direct copy of resolve from connectTo, however SockAddr is never actually used outside of this context. The only reason to keep this tuple would be to avoid any additional divergence in this copied function.

Suggested change
return (sock, addrAddress addr)
return sock

Copy link
Contributor

@eborden eborden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the socket bits here look totally reasonable. I would hope that we can upstream some of this, but I won't hold my breath. I'm good to merge with green tests.

@pbrisbin
Copy link
Member

Going to migrate this into a new PR as our test suite doesn't work on forks (needs secrets for the test faktory image).

Thanks for the contribution!

@pbrisbin
Copy link
Member

Migrated to #89

@pbrisbin pbrisbin closed this Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants