Skip to content

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