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 auto-reconnect option #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dvbmgr
Copy link

@dvbmgr dvbmgr commented Aug 29, 2020

Hi,

Please find my first pull request to this project. It aims to enable the option to automatically try to reconnect to the LDAP server if the connection is closed. As I already mentionned in an issue, I am quite new to Elixir, so feel free to adapt this PR to what suits best.

In particular, if the new configuration retry_on_connection_closed is set to true, then each call to GenServer.call is observed by do_call_or_retry/2. If it returns a {:error, :ldap_closed} or a {:error, {:gen_tcp_error, term}} , then it calls Paddle.reconnect and resends the request.

I have not been able to find an equivent of :gen_tcp_error for the :ssl-module, but I think, based on eldap.erl's do_send/3, that both (maybe erroneously) return :gen_tcp_error.

I sadly wasn't able to reproduce the configuration for the tests, but I don't expect any to fail. But that would be something to check for in the QA.

It is tricky to write tests for this PR, and I wasn't really able to do so. My approach to ensure a correct behaviour:

  • for :ldap_closed errors, that is, when :eldap is able to close the connection properly, I just killed the socket using ss. It would be quite easy to implement, but I wasn't comfortable with adding this as a dependency, as it would be quite hard to setup for the users anyway.
  • for :gen_tcp_error, it's quite tricky, and I just played with my LDAP server a little bit. What should really be done in order to properly insure that it behaves as expected would be using a tool that allows for connections drop, as for instance toxiproxy does. But that's a lot of work, that I'm not willing to put in right now, and again, there is the extra dependency issue.

With my best regards !

@dvbmgr
Copy link
Author

dvbmgr commented Aug 29, 2020

Actually, I should probably update the specs of each function that calls do_call_and_retry too. I'll fix that in the next few days.

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.

1 participant