search

nats-io / elixir-nats

Elixir NATS client
MIT License
76 stars 14 forks source link

Fixes for upgrade to Elixir 1.7.4 #48

Closed ryancurtin closed 5 years ago

ryancurtin commented 5 years ago

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!

coveralls commented 5 years ago

Coverage Status

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

ryancurtin commented 5 years ago

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 commented 5 years ago

@ryancurtin Any estimations on when will it be merged ?

ryancurtin commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.