Support for automatic reconnection attempts after server goes away #66

mbaynton · 2017-06-14T15:28:59Z

From my requests initially made in issue #59, this PR adds support for retrying the connection.

…f requested in configuration options.

heiglandreas · 2017-06-14T15:35:52Z

Thank you for your contribution! 🎉

I'll try to go through it this weekend!

… occurring in Vagrant.

…urring on the PHP7.0 test only

mbaynton · 2017-06-15T14:16:01Z

Ok, that's as far as I think I'll go with this until I hear from you, @heiglandreas . I am suspicious that coveralls' algorithm has changed rather than that there is actually a decrease in coverage. (Look at others' PRs as well, everything is supposedly decreasing by 0.7%)

ThaDafinser · 2017-06-16T13:39:53Z

Isn't this a bit dangerous? If the reconnect "count" goes up, you should at least know it? (logging)
Since then you should check the root of the problem

mbaynton · 2017-06-16T14:11:14Z

@ThaDafinser, maybe for some sites? I suspect the most common cause for needing to reconnect in the wild will be the one I'm experiencing: we have a number of scripts that will bind and maybe run one search, and then hang out for a while while waiting for user input or doing other processing, and then try to query the ldap server again on the same socket, which the server has timed out. Nothing unexpected or extraordinary happening in that scenario, so I don't want a log entry about it.

If a site wanted to keep a close eye on client/server ldap connection reliability, both the server and the openldap client libs underlying the php implementation can be configured to produce those logs for you to chew on.

Hypothetically, if one wanted to add this to zend-ldap, I'm not seeing a logging interface already utilized by the codebase to hook into. The site desiring this feature could extend Ldap with a class aware of that sites' logging infrastructure and override shouldReconnect().

ThaDafinser · 2017-06-16T15:49:09Z

and then hang out for a while while waiting for user input or doing other processing

i'm not sure if it's good to do it that way generally, but maybe there is no other way for your special use case.

But in that use case i understand why you don't require logging for that.

mbaynton · 2017-09-19T20:37:00Z

@heiglandreas just a friendly request to take a look at these PRs if you get a chance. I'm adding my git repo to composer for now. Thanks!

heiglandreas · 2017-09-20T06:55:20Z

@mbaynton So this PR adds the ability to reconnect transparently to an LDAP-Server. What I don't quite understand is - under what circumstances does that happen? The default lifetime of a request usually is so short, that such a timeout shouldn't happen at all. So what is your explicit usecase for this? Having the changes in the library causes us to maintain it for some time and I'd like to get to know what benefit it brings…

heiglandreas

Please don't change existing tests but instead introduce new ones. The existing tests need to pass as they are otherwise we have BC-issues which we don't want.

heiglandreas · 2017-09-20T07:26:56Z

test/AbstractOnlineTestCase.php

+    }
+
+    protected static function getStandardOptions()
+    {


You should never change existing tests.That can easily introduce BC issues. Please write a new test that tests the new behaviour. The current behaviour should still be working AS-IS and that is tested by these unchanged tests…

Looks like the reason here was because I needed the options array in my ReconnectTest class. I will duplicate this code in ReconnectTest instead.

heiglandreas · 2017-09-20T07:28:31Z

test/OfflineTest.php

-        $ldap_mod_del = $this->getFunctionMock('Zend\\Ldap', 'ldap_mod_add');
-        $ldap_mod_del->expects($this->once())
+        $ldap_mod_add = $this->getFunctionMock('Zend\\Ldap', 'ldap_mod_add');
+        $ldap_mod_add->expects($this->once())


Please do not change existing tests. Especially when the test has nothing to do with the newly added features.

Oh, I see what this was. testAddingAttributesFails() had clearly been copied from testRemovingAttributesFails(), but the $ldap_mod_del local variable name hadn't been updated. Was just trying to tidy up a bit; it wouldn't have a material impact on the test, but I will revert it.

heiglandreas · 2017-09-20T07:29:34Z

test/OfflineTest.php

+        BuiltinFunctionMocks::$ldap_set_option_mock->enable();
+    }
+
+    public function tearDown()


Don't change the existsing test. If you need special stuff for new tests you should move them to a separate test-class

mbaynton · 2017-09-20T13:21:00Z

@heiglandreas Your concern about added maintenance makes sense if you do not see the utility of the change, so thanks for asking me about that. I'd say that it's not the lifetime of a request which is potentially problematic, but rather the length of time between requests while the TCP connection remains open and unused, during which an ldap server might rightly decide to terminate the bind and close the connection. Without an abstraction layer like zend-ldap that's a really annoying possibility to have to check for and handle every time you want to touch ldap.

It's the same general deal as a connection to an SQL database that is periodically queried, and presumably the impetus for eg. this module. Of course, with php being primarily used on the web, the lifetime of many entire php script executions is so short that this doesn't apply, but php is perfectly suited to general purpose scripting tasks, and I'd say its popular libraries such as zend-ldap ought to be up for the task too. It's in non-web contexts where I frequently encounter this issue.

Python seems to be more popular for general purpose scripting so I thought perhaps they'd already implemented this. I'm not a python guy but it took me only about 5 minutes to find this feature in pyldap: https://github.com/pyldap/pyldap/blob/630955779e90b80f7f04763df1ebb0196af269eb/Lib/ldap/ldapobject.py#L1022

I'll take a look at the tests later today, it's been long enough that I don't recall immediately why I was tinkering with them.

heiglandreas · 2017-09-20T13:29:22Z

Hey @mbaynton. Thanks for your clarification! I was already assuming that it's an issue within long-running scripts. While it's perfectly sensible to use PHP within such contexts it's not the main domain of PHP. And I'm not sure whether we should introduce a solution to a problem the zend-framework wasn't built for in the first place. But perhaps @weierophinney can shed some light there.

But even when we decide to not make that a use-case for zend-ldap, it would be great to at least have a way to notify the calling code (by pushing a notification or by the code being able to pull the information) that the connection was closed/terminated/timed out. So having at least that would be a great possibility.

mbaynton · 2017-09-20T13:53:34Z

@heiglandreas gotcha. I confess, I'm a Symfony developer ;) so I'll defer to you on what the Zend framework was built for, but on the surface it looks like non-web usage is also encouraged?

...I'd have gone with a Symfony component for ldap if there was one remotely as developed or mature as zend-ldap. But you're the only game in town per-se, so I hope it can be incorporated here.

weierophinney · 2017-09-20T15:17:35Z

And I'm not sure whether we should introduce a solution to a problem the zend-framework wasn't built for in the first place. But perhaps @weierophinney can shed some light there.

ZF components target a variety of contexts, based on the use cases presented. For instance, zend-http's client supports timeouts and retries, as does zend-feed's Reader subcomponent; zend-db allows for persistent connections.

I think supporting reconnects would be great. Logging can be added pretty easily by extending the client and overriding the methods where it occurs to introduce logging. I think having some sort of documentation demonstrating that would be ideal when providing this feature.

The only reason I would be reluctant to support the feature is if it poses a heavy maintenance burden; as such, I'll defer to @heiglandreas 's judgment; my main point is I do not see a conflict with what we support in other components.

heiglandreas · 2017-09-20T16:34:22Z

Thanks @weierophinney for the feedback.

So let's tackle this then! @mbaynton if you could have a look for the tests that would be great! Ad don't worry about the code-coverage…

heiglandreas

All in all a lot of changes that add a lot of stuff to the already rather lengthy LDAP-Class. Would moving that stuff into an extended ReconnectLdap-class be an option? (The name isn't really good, so when you find something more appropriate that would be much appreciated!)

heiglandreas · 2017-09-20T16:41:06Z

src/Ldap.php

@@ -92,7 +111,7 @@ public function __construct($options = [])
     */
    public function __destruct()
    {
-        $this->disconnect();
+        $this->unbind();


I understand that it does the same. But from a naming POV I'd definitely prefer the disconnect here.

I couldn't find any justification for why I changed this. Will remove the change.

heiglandreas · 2017-09-20T16:41:22Z

src/Ldap.php

+                $this->lastConnectBindParams[$method][$parameter],
+                $property
+            );
+            return self::coalesce(


What point am I missing here? Can this return be reached at all?

No, thanks for catching this!

I went for the version that includes the isset() check, but all possible values will always be set in this PR anyway (or I'd add another test that covers the unset case!). It becomes relevant when one attempts to merge this with PR#64 to support SASL binds.

heiglandreas · 2017-09-20T16:44:19Z

src/Ldap.php

                $this->boundUser = $username;
                return $this;
+            } else {


Isn't this else obsolete? When $bind was not false we returned and so there's no need for an else

I looked at this again but it looks right to me -- if $bind is false, the else provides an opportunity to try again if the user set a nonzero reconnectAttempts.

Here as well: Consider this code:

if ($bind !== false) { $this->boundUser = $username; return $this; } if ($this->shouldReconnect($this->resource)) { return $this; }

It does the same, as it allows an opportunity to try again when $bind is false. But without the else…

heiglandreas · 2017-09-20T16:47:35Z

src/Ldap.php


        throw $zle;
    }

+    protected function shouldReconnect($resource)
+    {
+        if ($this->reconnectCount < $this->getReconnectsToAttempt()


Could we reverse this if/else so that the else-condition will be checked first? That would allow us to return early and skip the else

Sorry I don't understand. There's only one conditional here, e.g. it's an else not an else if, so I'm not sure what you mean by the 'else'-condition. Did you want to check ldap_errno($resource) === -1 before $this->reconnectCount < $this->getReconnectsToAttempt()?

I was thinking along these lines:

protected function shouldReconnect($resource) { if ($this->reconnectCount >= $this->getReconnectsToAttempt() || ldap_errno($resource) !== -1 { $this->reconnectsAttempted = $this->reconnectCount; $this->reconnectCount = 0; return false; } $this->reconnectCount++; $this->reconnectSleep(); try { $this->connect(); $this->bind(); $this->reconnectsAttempted = $this->reconnectCount; $this->reconnectCount = 0; return true; } catch (LdapException $e) { if ($e->getCode() !== -1) { return false; } } return $this->shouldReconnect($this->getResource()); }

It's just shifting code around and altering the if-condition a bit but it cleans up indentation levels and also removes the not necessary elses…

heiglandreas · 2017-09-20T16:52:19Z

src/Ldap.php

+                $this->reconnectCount = 0;
+                return true;
+            } catch (LdapException $e) {
+                if ($e->getCode() === -1) {


What happens when there is an LDAPException with a code different to -1? we silently do nothing? Or should we rethrow the Exception? Alternatively perhaps throwing a new Exception would be an option as well…

Good question. As it's written I think the code "Does The Right Thing," but it is not very self-documenting. If you look at everywhere Ldap::shouldReconnect() is used, there was already code immediately following whatever ldap_*() function call that checks the return value and converts failures expressed in return values to exceptions.

I'll add an explicit return false; to Ldap::shouldReconnect() to make it clear that this is the intended outcome in the case of a code different from -1

mbaynton · 2017-09-21T19:21:00Z

PR #71 is needed for the build to pass.

heiglandreas

Please have a look at the two comments. If that code needs to be there, we have a backwards-compatibility issue I'd like to avoid! Please see whether that can be resolved!

heiglandreas · 2017-09-21T20:35:56Z

test/OfflineTest.php

@@ -80,6 +80,7 @@ public function testOptionsGetter()
                                 'useStartTls'            => false,
                                 'optReferrals'           => false,
                                 'tryUsernameSplit'       => true,
+                                 'reconnectAttempts'      => 0,


Does that really need to be set? Because if there's no fallback and that needs to be set that is a Backwards incompatibility! And I'd like to avoid that!

This shouldn't create a BC breakage. Ldap::getOptions() has always returned the current values of all supported options, but you don't have to pass anything new to the constructor or setOptions().

heiglandreas · 2017-09-21T20:36:03Z

test/OfflineTest.php

@@ -109,6 +110,7 @@ public function testConfigObject()
                                 'useStartTls'            => false,
                                 'optReferrals'           => false,
                                 'tryUsernameSplit'       => true,
+                                 'reconnectAttempts'      => 0,


Does that really need to be set? Because if there's no fallback and that needs to be set that is a Backwards incompatibility! And I'd like to avoid that!

mbaynton · 2017-09-22T16:03:14Z

Hey @heiglandreas did you see my response re: breaking changes? Do you understand or disagree?

heiglandreas · 2017-09-22T17:02:47Z

Seen it and agree! On the go to phpsouthafrica ATM so I‘ll get into it in more detail tomorrow or the day after! ;)

mbaynton · 2017-09-25T14:02:54Z

src/Ldap.php

+                $this->reconnectCount = 0;
+                return true;
+            } catch (LdapException $e) {
+                if ($e->getCode() === -1) {


Good question. As it's written I think the code "Does The Right Thing," but it is not very self-documenting. If you look at everywhere Ldap::shouldReconnect() is used, there was already code immediately following whatever ldap_*() function call that checks the return value and converts failures expressed in return values to exceptions.

I'll add an explicit return false; to Ldap::shouldReconnect() to make it clear that this is the intended outcome in the case of a code different from -1

mbaynton · 2017-09-25T14:04:28Z

src/Ldap.php

+            $isAdded = ldap_add($resource, $dn->toString(), $entry);
+            ErrorHandler::stop();
+        } while ($isAdded === false && $this->shouldReconnect($resource));
+
        if ($isAdded === false) {


This is an example of why we don't need to throw anything in shouldReconnect()

Curious, I'm not sure why this ended up being displayed here, it isn't about the change to the tests.

heiglandreas

Consider the two code examples I added to perhaps make clearer what I was thinking about…

heiglandreas · 2017-09-25T16:46:44Z

src/Ldap.php


        throw $zle;
    }

+    protected function shouldReconnect($resource)
+    {
+        if ($this->reconnectCount < $this->getReconnectsToAttempt()


I was thinking along these lines:

protected function shouldReconnect($resource) { if ($this->reconnectCount >= $this->getReconnectsToAttempt() || ldap_errno($resource) !== -1 { $this->reconnectsAttempted = $this->reconnectCount; $this->reconnectCount = 0; return false; } $this->reconnectCount++; $this->reconnectSleep(); try { $this->connect(); $this->bind(); $this->reconnectsAttempted = $this->reconnectCount; $this->reconnectCount = 0; return true; } catch (LdapException $e) { if ($e->getCode() !== -1) { return false; } } return $this->shouldReconnect($this->getResource()); }

It's just shifting code around and altering the if-condition a bit but it cleans up indentation levels and also removes the not necessary elses…

heiglandreas · 2017-09-25T16:50:14Z

src/Ldap.php

                $this->boundUser = $username;
                return $this;
+            } else {


Here as well: Consider this code:

if ($bind !== false) { $this->boundUser = $username; return $this; } if ($this->shouldReconnect($this->resource)) { return $this; }

It does the same, as it allows an opportunity to try again when $bind is false. But without the else…

mbaynton · 2017-09-26T18:55:45Z

@heiglandreas I see what you mean and your preferred way to express the logic is fine with me; I've updated it.

So about the automated tests, it's failing against PHP 5.6 because you need to approve PR#71 ;), but my theory about the failures on PHP 7.0 is that's the one running coverage analysis, and it is slowing down enough to actually experience a timeout with the low value I configured in slapd.conf :(. Ick.

Maybe run two separate slapd server instances in the CI environment, one on a different port used only by the reconnect tests? Got any better ideas?

Edit 9/27: It looks like this tool combined with a kernel from late 2015 or better could target and close the specific socket, forcing the reconnect logic to be tested without relying on timeouts. This would require moving to a very up-to-date version of ss and the kernel; for example it's not in Ubuntu 16.04.

mbaynton · 2017-09-29T19:42:44Z

@heiglandreas, the network administrator at my office and I figured out another way to produce connectivity issues that force the reconnection code to be exercised. It does require some notable changes to the CI / Vagrant environments though, such as

TravisCI requires sudo
Another port forward is created in Vagrant, and the php built-in webserver listens on it as a simple channel for signaling when to produce ldap connection dropouts.

Have a look at https://github.com/mbaynton/zend-ldap/pull/1/files for all the changes. If this looks good to you I'll merge them to this PR.

It does result in completely passing tests.

mbaynton · 2017-10-09T18:49:27Z

Hey @heiglandreas. I'd hate to lose momentum on this PR. Want me to merge my CI setup changes in so the tests pass?

heiglandreas · 2017-10-18T05:16:43Z

Hey @mbaynton. I'd hate to loose the containerized infrastructure on travis for this project. But the changes needed for testing the dropped connection seem too need this.

Though at the moment I'm much more concerned about the failing tests due to the missing or "wrong" LDAP-Extension…

mbaynton · 2017-10-18T13:27:38Z

@heiglandreas Yes, you're right about not being able to use a container-based environment with Travis. Can't be helped as far as I know though.

About the ldap.so warnings, that's not causing the tests to fail and also not related to this PR. See for example the other PR for ldap_mod_replace, whose test run produces the same warnings but is passing.

I'll go ahead and merge my CI changes into this PR so we can see a passing test run.

Rework reconnect tests to cause established connections to actively be dropped, rather than waiting on a timeout.

heiglandreas · 2017-10-18T13:39:07Z

Hey Mike. Perfect! Thanks!

It was far too early this morning…

mbaynton · 2017-11-06T21:52:13Z

@heiglandreas, by the way about those ldap.so warnings, just noticed I had already actually tracked down and fixed the ldap.so warnings in the Travis-CI builds, if you wanna go ahead and merge #65 ;) Or this one. That'd be cool too.

mbaynton · 2018-06-27T18:50:36Z

@heiglandreas, looks like where we left off with this the changes were approved and the tests were passing. Is there anything more you need from me to help the process of getting it merged?

Support for automatic reconnection attempts after server goes away

heiglandreas · 2018-07-04T16:01:56Z

I've found the issue! The PR introduces a firewall for testing purposes. But that firewall was missing the rule to allow traffic through the LDAPS-port. That wasn't necessary at the time you created it… 😄

Support for automatic reconnection attempts after server goes away

This is necessary to make use of the reconnect-possibilities introduced with #66 The code is taken from PR #69

Support for automatic reconnection attempts after server goes away, i…

abcd64b

…f requested in configuration options.

mbaynton added 3 commits June 14, 2017 15:31

Fix test failures.

87d7da5

Some changes to likely fix failures occurring in Travis that were not…

77199fe

… occurring in Vagrant.

Fix CS violations, another attempt to squash a connection failure occ…

3d1f23d

…urring on the PHP7.0 test only

heiglandreas suggested changes Sep 20, 2017

View reviewed changes

mbaynton added 2 commits September 21, 2017 11:05

Refactor tests to minimize impact on existing test classes.

078cf83

CS

8cf3971

heiglandreas suggested changes Sep 21, 2017

View reviewed changes

heiglandreas approved these changes Sep 25, 2017

View reviewed changes

mbaynton commented Sep 25, 2017

View reviewed changes

Changes per @heiglandreas code review

c6a5b80

heiglandreas reviewed Sep 25, 2017

View reviewed changes

mbaynton added 2 commits September 26, 2017 13:13

More changes per @heiglandreas code review

2b322fc

CS

2862ee9

Merge branch 'master' into reconnect

142ad2b

Reconnect ci fix (#1)

f25f183

Rework reconnect tests to cause established connections to actively be dropped, rather than waiting on a timeout.

heiglandreas pushed a commit to heiglandreas/zend-ldap that referenced this pull request Jul 3, 2018

Merge pull request zendframework#66 from mbaynton/reconnect

eee74d8

Support for automatic reconnection attempts after server goes away

heiglandreas added a commit to heiglandreas/zend-ldap that referenced this pull request Jul 4, 2018

Merge pull request zendframework#66 from mbaynton/reconnect

2e526ca

Support for automatic reconnection attempts after server goes away

heiglandreas mentioned this pull request Jul 4, 2018

Use Ubuntu Trusty for Travis CI runs #65

Closed

heiglandreas added a commit that referenced this pull request Jul 5, 2018

Add reconnect-attempts

700ff73

This is necessary to make use of the reconnect-possibilities introduced with #66 The code is taken from PR #69

heiglandreas self-assigned this Jul 5, 2018

heiglandreas added this to the 2.10.0 milestone Jul 5, 2018

heiglandreas closed this in 5fc468a Jul 5, 2018

Support for automatic reconnection attempts after server goes away #66

Support for automatic reconnection attempts after server goes away #66

Conversation

mbaynton commented Jun 14, 2017

heiglandreas commented Jun 14, 2017

mbaynton commented Jun 15, 2017 • edited Loading

ThaDafinser commented Jun 16, 2017

mbaynton commented Jun 16, 2017 • edited Loading

ThaDafinser commented Jun 16, 2017

mbaynton commented Sep 19, 2017

heiglandreas commented Sep 20, 2017

heiglandreas left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaynton commented Sep 20, 2017

heiglandreas commented Sep 20, 2017

mbaynton commented Sep 20, 2017

weierophinney commented Sep 20, 2017

heiglandreas commented Sep 20, 2017

heiglandreas left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaynton Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

mbaynton commented Sep 21, 2017

heiglandreas left a 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

mbaynton commented Sep 22, 2017

heiglandreas commented Sep 22, 2017

mbaynton Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heiglandreas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaynton commented Sep 26, 2017 • edited Loading

mbaynton commented Sep 29, 2017

mbaynton commented Oct 9, 2017

heiglandreas commented Oct 18, 2017

mbaynton commented Oct 18, 2017

heiglandreas commented Oct 18, 2017

mbaynton commented Nov 6, 2017

mbaynton commented Jun 27, 2018

heiglandreas commented Jul 4, 2018

mbaynton commented Jun 15, 2017 •

edited

Loading

mbaynton commented Jun 16, 2017 •

edited

Loading

mbaynton Sep 25, 2017 •

edited

Loading

mbaynton Sep 25, 2017 •

edited

Loading

mbaynton commented Sep 26, 2017 •

edited

Loading