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

Fix socket options for newest elixir-socket package #7

Merged
merged 1 commit into from
Mar 27, 2017

Conversation

edennis
Copy link
Contributor

@edennis edennis commented Mar 13, 2017

Here's a fix for issue #6. The problem was that the option handling was sneakily changed in this innocuous "fix warnings" commit to elixir-socket. Now, instead of silently ignoring any unknown options to Socket.connect it barfs when the pattern match fails. The handling of send_timeout was also changed so that now an embedded keyword list is needed. The other thing I noticed is that the deliver: : term option was never passed through to :inet.setopts/2 even in the earlier versions of elixir-socket so I removed it.

Just as a side note: I didn't have a lot of confidence in this library after taking a closer look at the code and the complete absence of tests wasn't very convincing either. It may be worth considering whether or not it's worth cutting the dependency completely and just using gen_tcp directly. Thoughts?

* Updated elixir-socket in mix.exs
@MaxPower15 MaxPower15 merged commit 4d85222 into wistia:master Mar 27, 2017
@MaxPower15
Copy link
Member

MaxPower15 commented Mar 27, 2017

Thanks! Nice catch.

RE: the socket package, I have similar concerns. The main thing it does for us now is tls negotiation, but if we can do that ourselves, depending on fewer packages is a plus. Especially if we're shedding packages without tests.

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

2 participants