Add layer on top of LDAP methods to catch server disconnects and attempt re-connection #4992
Conversation
…mpt a one time reconnect.
|
@bline, thanks for your PR! By analyzing the history of the files in this pull request, we identified @blizzz, @LukasReschke and @MorrisJobke to be potential reviewers. |
|
I discovered today this pull request breaks calls to controlPagedResultResponse because $cookie is passed by reference. After some research it seems there is no simple work-around for passing by reference using call_user_func_array(). The only solution I've come up with so far is to make a special case in invokeLDAPMethod for controlPagedResultResponse. Any suggestions? |
…e was a generic way to pass by reference with call_user_func_array..
Codecov Report
@@ Coverage Diff @@
## master #4992 +/- ##
============================================
- Coverage 54.16% 54.16% -0.01%
- Complexity 22276 22281 +5
============================================
Files 1379 1379
Lines 85301 85321 +20
Branches 1322 1322
============================================
+ Hits 46203 46213 +10
- Misses 39098 39108 +10
|
|
OK, this last commit is less invasive, still passing CR around and not changing public method. I've done more testing and it no longer breaks controlPagedResultResponse pass by reference call. Any input welcome. Thanks! |
MorrisJobke
added
3. to review
|
@bline thansk for looking into it. Just diving into your code. Up front:
I had the same issue when i wrote the LDAP Wrapper, there is also calls the method directly instead of using the invoker method 🙈 https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/LDAP.php#L74 |
|
On my local test case it is not working so far. Connection gets re-established, but I am only getting an empty result. |
| if(!$this->ldap->isResource($cr)) { | ||
| // Seems like we didn't find any resource. | ||
| \OCP\Util::writeLog('user_ldap', "Could not $command, because resource is missing.", \OCP\Util::DEBUG); | ||
| return false; |
|
It looks like somehow my OpenLDAP instance succeeds with a bind on reconnect, but does not operate. Need to leave now, will debug later. |
|
So the problem is that methods like executeSearch re-use the old LDAP resource, instead of fetching the newly created one. Now we could change this everywhere so the link is always fetched, but this is error prone. Just waits for a future PR where this is forgotten. The really right thing to do would be to rip this out of Connection and Access into a Client that is fed with the initial server and bind parameters and otherwise does the talking to LDAP. Partly, the LDAP Wrapper is doing this already. It also could take over the resource-management, and neither Access nor Connection should have anything to do with it. Or we indeed create a Client class that extends the LDAP Wrapper. This also satisfies my urge to have this handling out of Access (which is already too big anyway). |
Extending sucks, as the method signatures don't fit us. All the methods would be doubled and the usages needs to be adjusted either way. Making LDAP also do the connection handling seems better. Although there is also some overhead that needs to go into it (e.g. cloning behaviour). All in all this is also a hell to backport :( Best we go for the less invasive but not future-proof solution for <= stable12 and only to the architectural changes for master. |
|
I created a new PR in a branch where we can work together: #5104 and pushed my changes that fixes the issues I found. Let's follow up there. |
This pull request adds a wrapper to calls to LDAP.php in order to trap server disconnects and attempt a reconnect. This happens on long running processes such as nextant solr indexer when the AD server times out the connection. This solves nextcloud/fulltextsearch#175 for me.