DRY up connection handling logic #224

javanthropus · 2015-09-29T03:01:08Z

This patch simplifies configuration of new connections and removes some duplicate code around setting up encryption. However, it does change the exception messages somewhat. All the same exception types are raised in all the same places, but mostly the messages of underlying exceptions that Net::LDAP::Error and Net::LDAP::ConnectionRefusedError effectively wrap are used directly rather than providing new, different messages. These changes don't break any tests, but it's possible that some user of the library may depend on the specific messages currently produced.

The upshot of this change of exception messages will mostly be felt when users provide host lists. If all given hosts fail, each failure message for each host is included in an exception summarizing all of the failures along with the original exception type and host parameters that triggered the failure message. This will certainly help when debugging failures in the multiple host case.

jch · 2015-09-29T16:28:04Z

lib/net/ldap/connection.rb

  end

-  def open_connection(server)
+  def open_connection(hosts, encryption)


I know this is a private class, but why change the method signature for open_connection? All of the information is available in the previous server hash.

This is mostly to make it explicit what arguments these methods actually use. Passing around a hash makes it necessary to inspect the guts of the method to know what is really needed.

👍 that's a good reason, but could you revert that and open it in a follow up PR?

It makes it easier to revisit merged PR's linked from the changelog.

I can do that. Which would you like to tackle first, the PR that simplifies parameter handling or this one?

Your call. Whatever's easier for you.

jch · 2015-09-29T16:33:01Z

The upshot of this change of exception messages will mostly be felt when users provide host lists. If all given hosts fail, each failure message for each host is included in an exception summarizing all of the failures along with the original exception type and host parameters that triggered the failure message

This sounds like a worthwhile goal, but your PR is also trying to clean up code at the same time. Could you minimize your changeset to focus on this grouping of errors?

jch · 2015-09-29T17:09:30Z

lib/net/ldap/connection.rb

-      open_connection(server)
+      hosts = server[:hosts]
+      hosts = [[server[:host], server[:port]]] if hosts.nil?
+      open_connection(hosts, server[:encryption])


I should've suggested this in your previous PR, but what do you think of this idea: Instead of having open_connection worry about handling multiple hosts and then checking whether it did one host, or multiple, we can iterate through server[:hosts] here. The advantage is that open_connection would only be responsible for what it's method name suggests: opening a single connection. You can then collect the errors here and handle it in a generic way without the code smell we're discussing below. It'd also minimize remove the changes from open_connection, which I think make the method less readable.

I think that's a good idea. I'll look into it.

After looking into this, I think it won't work out. Most of what open_connection does in this patch is manage exception dispatching, and it needs to do this because in the multi-host case it must suppress any exceptions raised if a connection is successfully opened. Opening the connection is actually handled by prepare_socket and TCPSocket.new, one line in the open_connection method. The rest of the method is the looping over hosts to try, gathering of exceptions, and finally raising an exception based on the captured exceptions if no connection was opened.

In other words, to do what you suggest moves about 19 lines from open_connection into initialize, leaving open_connection as a one-liner and initialize rather more bloated. I don't really see how this would address the exception handling code smell either since we're just moving logic around rather than eliminating or refactoring it significantly.

Have you looked at the result of the patch rather than the diffs yet? I think you'll see that the patch is noisy only because it's simplifying the code.

javanthropus · 2015-09-30T23:00:31Z

Before I start working on things here, it sounds like you would like to break this into 3 PRs: 1 that changes parameter handling in the open_connection and prepare_socket methods, 1 that reworks exception handling, and then whatever is left goes into this one. Does that sound right?

jch · 2015-09-30T23:18:59Z

Just one PR for changing the method signature, and the rest in this PR is fine.

javanthropus · 2015-09-30T23:20:47Z

Cool deal. I'll see what I can do this evening.

* ConnectionError wraps the creation of several error types for backward compatibility * For now, ConnectionError is only created when more than 1 error is given to the constructor * In the future, ConnectionError should be used even in the single error case

javanthropus · 2015-10-18T23:16:43Z

@jch I have rewritten the branch for this pull request to hopefully address the main concerns raised regarding exception handling and method signature changes. We can't really escape the code smell of the exception handling here, but it should be easier to clean up in the future if we allow for slightly different exceptions to be raised for the single host connection attempt.

jch · 2015-10-19T16:36:38Z

lib/net/ldap/connection.rb

+  rescue
+    # Ensure the connection is closed when requested in the event of an SSL
+    # setup failure.
+    @conn.close if close


This seems like new behavior. Why this change in this PR?

Sockets created by the library in open_connection need to be closed in the event that they cannot successfully be configured, such as for encryption. Otherwise, we leak file descriptors and hope that the GC releases them in a timely manner. Not good, especially in the multiple host case where we may fail to configure the sockets for numerous hosts in rapid succession.

This can also be handled within the open_connection method since that is the only place where we currently set the close parameter. Now that you bring it up, I think I like this option better, so I'll make this change.

jch · 2015-10-19T16:41:48Z

Thanks for picking this up again

javanthropus · 2015-10-21T21:29:00Z

@jch What do you think about these latest changes?

jch · 2015-10-21T21:55:12Z

lib/net/ldap/connection.rb


    if server[:socket]
      prepare_socket(server)
    else
+      server[:hosts] = [[server[:host], server[:port]]] if server[:hosts].nil?


Note: This works even when server[:host] and/or server[:port] are nil. It raises a SocketError and matches up with the old behavior.

jch · 2015-10-21T21:58:04Z

@javanthropus thanks for the ping. I added one last piece of small feedback. We're getting close!

javanthropus · 2015-10-27T14:50:51Z

@jch Sorry for the delay on getting the last changes in. Let me know how this looks.

DRY up connection handling logic

jch · 2015-10-27T16:41:48Z

@javanthropus great work, thanks!

=== Net::LDAP 0.12.1 * Whitespace formatting cleanup {#236}[ruby-ldap/ruby-net-ldap#236] * Set operation result if LDAP server is not accessible {#232}[ruby-ldap/ruby-net-ldap#232] === Net::LDAP 0.12.0 * DRY up connection handling logic {#224}[ruby-ldap/ruby-net-ldap#224] * Define auth adapters {#226}[ruby-ldap/ruby-net-ldap#226] * add slash to attribute value filter {#225}[ruby-ldap/ruby-net-ldap#225] * Add the ability to provide a list of hosts for a connection {#223}[ruby-ldap/ruby-net-ldap#223] * Specify the port of LDAP server by giving INTEGRATION_PORT {#221}[ruby-ldap/ruby-net-ldap#221] * Correctly set BerIdentifiedString values to UTF-8 {#212}[ruby-ldap/ruby-net-ldap#212] * Raise Net::LDAP::ConnectionRefusedError when new connection is refused. {#213}[ruby-ldap/ruby-net-ldap#213] * obscure auth password upon #inspect, added test, closes #216 {#217}[ruby-ldap/ruby-net-ldap#217] * Fixing incorrect error class name {#207}[ruby-ldap/ruby-net-ldap#207] * Travis update {#205}[ruby-ldap/ruby-net-ldap#205] * Remove obsolete rbx-19mode from Travis {#204}[ruby-ldap/ruby-net-ldap#204] * mv "sudo" from script/install-openldap to .travis.yml {#199}[ruby-ldap/ruby-net-ldap#199] * Remove meaningless shebang {#200}[ruby-ldap/ruby-net-ldap#200] * Fix Travis CI build {#202}[ruby-ldap/ruby-net-ldap#202] * README.rdoc: fix travis link {#195}[ruby-ldap/ruby-net-ldap#195]

…handling DRY up connection handling logic

jch reviewed Sep 29, 2015
View reviewed changes

Jeremy Bopp added 3 commits October 2, 2015 15:54

DRY up connection handling logic

d5f7516

Resolve rubocop violations

e1a0d13

javanthropus force-pushed the dry_up_connection_handling branch from 085a622 to e1a0d13 Compare October 18, 2015 23:13

jch reviewed Oct 19, 2015
View reviewed changes

Jeremy Bopp added 2 commits October 19, 2015 14:42

Assign exceptions to a variable rather than use $!

5a4ddb1

Close the socket where opened when necessary

ab320af

jch reviewed Oct 21, 2015
View reviewed changes

Move connection cleanup logic into the close method

e829069

jch added a commit that referenced this pull request Oct 27, 2015

Merge pull request #224 from javanthropus/dry_up_connection_handling

4b22ea7

DRY up connection handling logic

jch merged commit 4b22ea7 into ruby-ldap:master Oct 27, 2015

javanthropus deleted the dry_up_connection_handling branch October 27, 2015 20:06

astratto pushed a commit to astratto/ruby-net-ldap that referenced this pull request Dec 18, 2015

Merge pull request ruby-ldap#224 from javanthropus/dry_up_connection_…

6139081

…handling DRY up connection handling logic

dependabot-preview bot mentioned this pull request Nov 1, 2018

Update net-ldap requirement from = 0.11 to = 0.16.1 sensu-plugins/sensu-plugins-openldap#9

Closed

dependabot-preview bot mentioned this pull request Jan 25, 2019

Update net-ldap requirement from ~> 0.3.1 to ~> 0.16.1 malachaifrazier/redmine#7

Open

dependabot-preview bot mentioned this pull request Jun 6, 2019

[Security] Bump net-ldap from 0.3.1 to 0.16.1 CUGC/nyyms#25

Closed

dependabot bot mentioned this pull request Jul 2, 2019

Bump net-ldap from 0.11 to 0.16.0 in /script/ldap_test/2015-02-09 fiedl/your_platform#34

Merged

dependabot bot mentioned this pull request Oct 30, 2019

Bump net-ldap from 0.10.1 to 0.16.0 wogri/inki#5

Merged

dependabot bot mentioned this pull request Dec 17, 2019

Bump net-ldap from 0.7.0 to 0.16.0 berkmancenter/brkmn#8

Closed

dependabot bot mentioned this pull request Jan 24, 2020

Bump net-ldap from 0.3.1 to 0.16.0 akusumoto/kredmine#2

Closed

This was referenced Mar 4, 2020

Bump net-ldap from 0.3.1 to 0.16.0 INRIA/osc#3

Open

Bump net-ldap from 0.11 to 0.16.0 Bwian/pops#146

Closed

dependabot-preview bot mentioned this pull request Nov 13, 2020

[Security] Bump net-ldap from 0.3.1 to 0.16.0 nditech/red-innovadores-lms#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRY up connection handling logic #224

DRY up connection handling logic #224

javanthropus commented Sep 29, 2015

jch Sep 29, 2015

javanthropus Sep 29, 2015

jch Sep 29, 2015

javanthropus Sep 29, 2015

jch Sep 29, 2015

jch commented Sep 29, 2015

jch Sep 29, 2015

javanthropus Sep 29, 2015

javanthropus Oct 1, 2015

javanthropus commented Sep 30, 2015

jch commented Sep 30, 2015

javanthropus commented Sep 30, 2015

javanthropus commented Oct 18, 2015

jch Oct 19, 2015

javanthropus Oct 19, 2015

jch commented Oct 19, 2015

javanthropus commented Oct 21, 2015

jch Oct 21, 2015

jch commented Oct 21, 2015

javanthropus commented Oct 27, 2015

jch commented Oct 27, 2015

DRY up connection handling logic #224

DRY up connection handling logic #224

Conversation

javanthropus commented Sep 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jch commented Sep 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanthropus commented Sep 30, 2015

jch commented Sep 30, 2015

javanthropus commented Sep 30, 2015

javanthropus commented Oct 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jch commented Oct 19, 2015

javanthropus commented Oct 21, 2015

Choose a reason for hiding this comment

jch commented Oct 21, 2015

javanthropus commented Oct 27, 2015

jch commented Oct 27, 2015