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

Fixes for upgrade to Elixir 1.7.4 #48

Closed
wants to merge 4 commits into from
Closed

Conversation

ryancurtin
Copy link

This PR makes a few fixes to enable developers to use Elixir 1.7.4. The main issue here was the usage of unsafe variables within case statements in connection.ex, which prevented the application from starting correctly if using a separate writer.

I also ran all the code through the formatter, which, admittedly makes it difficult to see my changes. I'm happy to submit a PR without the formatter, too. Please take a look and let me know what you think!

Removing unused dependency - only added to fix local compilation

Cleanup
@coveralls
Copy link

coveralls commented Dec 17, 2018

Coverage Status

Coverage increased (+22.4%) to 88.581% when pulling ea84bcf on OneCloudInc:master into c30ca2c on nats-io:master.

@ryancurtin
Copy link
Author

Note: It looks like the build is failing because of the to_charlist not being defined in Elixir 1.2.2. IIRC, this was removed in Elixir 1.5. Would you mind if I updated the CI environment to use the newest versions of Elixir? Since the reason for my fix was to support 1.7.4, I don't think we need to worry too much about backwards compatibility once a new version of this package is released.

@konstantinzolotarev
Copy link

@ryancurtin Any estimations on when will it be merged ?

@ryancurtin
Copy link
Author

@konstantinzolotarev - Unfortunately I'm not a maintainer of this project. I spent some time today getting the tests to pass in Travis, but it looks like there is a TLS error in OTP Version 20.3 that is causing some failures.

FWIW, I've been running my forked version in production for about 3 months without any problems: https://github.com/OneCloudInc/elixir-nats/releases/tag/0.1.5

@ryancurtin
Copy link
Author

@kozlovic - I wanted to bring your attention, as you previously helped me navigate an issue with nats-io/gnatsd. It looks like the maintainer is no longer in the nats-io organization. Will elixir-nats have official support? I don't know if I'll be able to commit to maintaining this in an official capacity, but I would gladly work with you or someone on your team to help get this PR merged.

@derekcollison
Copy link
Member

Thanks for the offer to help. Yes the maintainer is no longer supporting the work. I noticed some of the builds failed, could you look and see if these are flappers or are something more serious? Clean travis runs would be great.

@ryancurtin
Copy link
Author

@derekcollison - Thanks for getting back to me! I did some testing and it appears that the test is failing due to a TLS record overflow when attempting a TLS connection (the message from :ssl.connect was {:tls_alert, "record overflow"}. The issue only seems to be present in OTP 20.3 across all of the versions of Elixir I tested.

Based on the official support matrix for Elixir (https://github.com/elixir-lang/elixir/blob/master/lib/elixir/pages/Compatibility%20and%20Deprecations.md), it appears that OTP 20 is still very much a factor. I'd have to do more digging to really get to the bottom of it.

I'll be able to investigate in the background, but in the meantime I'd invite anyone else following to chime in. It would be a shame to have to release this without support for an OTP version that is still widely used.

@ColinSullivan1
Copy link
Member

Thank you for using NATS and for this contribution!

Note that this client has largely been unmaintained and so has been deprecated, so I'm closing this PR. Moving forward we suggest you migrate to the nats.ex client which will be actively maintained. We'll keep this repository public in case you'd prefer to make a copy to maintain yourselves, but hope you'll have a good experience with the new client. We sincerely apologize for the inconvenience.

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

5 participants