Re: LdapConnectionPool.getConnection doing extraneous search?
> On Mar 23, 2021, at 10:45 AM, Emmanuel Lécharny wrote:
>
> On 23/03/2021 14:24, Shawn McKinney wrote:
>>> On Mar 23, 2021, at 3:00 AM, Emmanuel Lécharny wrote:
>>>
>>>
>>> On 22/03/2021 19:41, Shawn McKinney wrote:
> On Mar 22, 2021, at 11:05 AM, Emmanuel Lécharny
> wrote:
>
> LDAP connections are quite stable. Again, this check is there to respect
> the commons-pool API contract. If the connection is dead, then doing this
> check will let the pool fetching another connection, which is good. OTOH,
> if you don't want to check connections while fetching one, then you are
> on your own (ie, deal with the consequences of a bad connection).
Again, I must disagree. Connections aren’t stable, subjected to env
conditions and we can never assume a connection is good.
>>>
>>> You *can* assume it's good, and react exceptionally if it's not. That the
>>> fastest way to deal with potential bad connections, if you want to avoid a
>>> check every time you pull a connection from the pool. (but see later for
>>> another option)
>> There are 150 locations in fortress core where an ldap connection is being
>> pulled from the pool. No way I want to revisit all of that code.
>>>
>>> Something in the API chain must do the job of testing and reconnect, every
>>> time it’s pulled from the pool.
>>>
>>> This is exactly what the testOnBorrow does ;-) But it's costly... (see
>>> later for another option)
>>>
Now, having said that, that’s exactly what I’m observing currently, with
the test on borrow flag turned off.
Let me explain the scenario:
1. Start server
2. Start client
This initializes a connection pool which creates and stores exactly 1
connection (min 1, max1 )
3. Set breakpoint in client on pool.getConnection method
4. Restart the server.
5. Client performs ldap op which triggers the breakpoint on getConnection
6. Server at this point still hasn’t any connections with the client. The
client ‘thinks’ it has connections in the pool, but these were broken on
step 4.
7. Step over the getConnection which then triggers the commons pool to
execute:
```
GenericObjectPool._factory.activateObject(latch.getPair().value)
```
8. A connection is made with the server, along with bind
```
6058e163 conn=1000 fd=14 ACCEPT from IP=127.0.0.1:35246 (IP=127.0.0.1:389)
[exec] 6058e163 conn=1000 op=0 BIND dn="cn=manager,dc=example,dc=com"
method=128
[exec] 6058e163 conn=1000 op=0 BIND dn="cn=manager,dc=example,dc=com"
mech=SIMPLE ssf=0
[exec] 6058e163 conn=1000 op=0 RESULT tag=97 err=0 duration=1.667ms
```
9. Client continues with its ldap op successfully and is never the wiser
that the connections were all forcibly closed on server restart.
This is EXACTLY what I want to see all of which is done without the test
on borrow eliminating the extraneous search on every connection retrieval.
>>>
>>> So that would mean we have some kind of 'retry' on connection operation: if
>>> it fails, then let's assume the connection is broken, and redo it with a
>>> fresh connection.
>>>
>> Yes, that’s what’s happening.
>> In the Commons pool lib, this is the block that gets executed:
>> ``` this._factory.activateObject(latch.getPair().value);
>> if (this._testOnBorrow &&
>> !this._factory.validateObject(latch.getPair().value)) {
>> throw new Exception("ValidateObject failed");
>> }
>> ```
>> In the first line above, activateObject, this code gets called, from our
>> AbstractPoolableLdapConnectionFactory class:
>> ```
>> public void activateObject(LdapConnection connection) throws LdapException {
>> LOG.debug("Activating {}", connection);
>> if (!connection.isConnected() ||
>> !connection.isAuthenticated()) {
>> LOG.debug("rebind due to connection dropped on {}", connection);
>> this.connectionFactory.bindConnection(connection);
>> }
>> ```
>> The code performs a rebind which renews the connection.
>> All of this with testOnBorrow being false!
>
>
> The second bind is pretty brutal, as it will create a brand new connection.
>
>> So, I’m still scratching my head figuring why we need this secondary level
>> that is wasting a round trip to the server.
>
> Well oh well, not sure we need it at all, considering the piece of code you
> exhumated ;-)
>
Yes, that’s pretty much where I’m at. I’ve changed fortress to turn it off by
default. Also be good to think about changing the default behavior in teh API
as well.
>
>>> The problem is that the pool is passive: it does not react to any
>>> connection event, like a connection closure. OTOH, when a connection is
>>> properly closed by the server (ie an NoticeOfDisconnect - aka NoD - is
>>> generated by the server), the connection should process it and die. Now we
>>> have an issue: the connection is just ly
Re: LdapConnectionPool.getConnection doing extraneous search?
On 23/03/2021 14:24, Shawn McKinney wrote:
On Mar 23, 2021, at 3:00 AM, Emmanuel Lécharny wrote:
On 22/03/2021 19:41, Shawn McKinney wrote:
On Mar 22, 2021, at 11:05 AM, Emmanuel Lécharny wrote:
LDAP connections are quite stable. Again, this check is there to respect the
commons-pool API contract. If the connection is dead, then doing this check
will let the pool fetching another connection, which is good. OTOH, if you
don't want to check connections while fetching one, then you are on your own
(ie, deal with the consequences of a bad connection).
Again, I must disagree. Connections aren’t stable, subjected to env conditions
and we can never assume a connection is good.
You *can* assume it's good, and react exceptionally if it's not. That the
fastest way to deal with potential bad connections, if you want to avoid a
check every time you pull a connection from the pool. (but see later for
another option)
There are 150 locations in fortress core where an ldap connection is being
pulled from the pool. No way I want to revisit all of that code.
Something in the API chain must do the job of testing and reconnect, every time
it’s pulled from the pool.
This is exactly what the testOnBorrow does ;-) But it's costly... (see later
for another option)
Now, having said that, that’s exactly what I’m observing currently, with the
test on borrow flag turned off.
Let me explain the scenario:
1. Start server
2. Start client
This initializes a connection pool which creates and stores exactly 1
connection (min 1, max1 )
3. Set breakpoint in client on pool.getConnection method
4. Restart the server.
5. Client performs ldap op which triggers the breakpoint on getConnection
6. Server at this point still hasn’t any connections with the client. The
client ‘thinks’ it has connections in the pool, but these were broken on step 4.
7. Step over the getConnection which then triggers the commons pool to execute:
```
GenericObjectPool._factory.activateObject(latch.getPair().value)
```
8. A connection is made with the server, along with bind
```
6058e163 conn=1000 fd=14 ACCEPT from IP=127.0.0.1:35246 (IP=127.0.0.1:389)
[exec] 6058e163 conn=1000 op=0 BIND dn="cn=manager,dc=example,dc=com"
method=128
[exec] 6058e163 conn=1000 op=0 BIND dn="cn=manager,dc=example,dc=com"
mech=SIMPLE ssf=0
[exec] 6058e163 conn=1000 op=0 RESULT tag=97 err=0 duration=1.667ms
```
9. Client continues with its ldap op successfully and is never the wiser that
the connections were all forcibly closed on server restart.
This is EXACTLY what I want to see all of which is done without the test on
borrow eliminating the extraneous search on every connection retrieval.
So that would mean we have some kind of 'retry' on connection operation: if it
fails, then let's assume the connection is broken, and redo it with a fresh
connection.
Yes, that’s what’s happening.
In the Commons pool lib, this is the block that gets executed:
``` this._factory.activateObject(latch.getPair().value);
if (this._testOnBorrow &&
!this._factory.validateObject(latch.getPair().value)) {
throw new Exception("ValidateObject failed");
}
```
In the first line above, activateObject, this code gets called, from our
AbstractPoolableLdapConnectionFactory class:
```
public void activateObject(LdapConnection connection) throws LdapException {
LOG.debug("Activating {}", connection);
if (!connection.isConnected() ||
!connection.isAuthenticated()) {
LOG.debug("rebind due to connection dropped on {}", connection);
this.connectionFactory.bindConnection(connection);
}
```
The code performs a rebind which renews the connection.
All of this with testOnBorrow being false!
The second bind is pretty brutal, as it will create a brand new connection.
So, I’m still scratching my head figuring why we need this secondary level that
is wasting a round trip to the server.
Well oh well, not sure we need it at all, considering the piece of code
you exhumated ;-)
The problem is that the pool is passive: it does not react to any connection
event, like a connection closure. OTOH, when a connection is properly closed by
the server (ie an NoticeOfDisconnect - aka NoD - is generated by the server),
the connection should process it and die. Now we have an issue: the connection
is just lying in the pool, not being used by any client, so there is a missing
step: removing the connection from the pool in this very case. That can be
something we can add to the current LDAP pool.
Note that if the server does not send a NoD, you are screwed. There is no way
to be sure that the connection is ok until you use it. OTOH, leveraging the
setTestWhileIdle() could be a solution to partially workaround the issue.
Here you’ve lost me. I’m running the server in debug mode, inside a bash
shell, running in the foreground.
In my test I stop the server by ‘ctrl-c’ killing the shell.
The server i
Re: LdapConnectionPool.getConnection doing extraneous search?
> On Mar 23, 2021, at 3:00 AM, Emmanuel Lécharny wrote:
>
>
> On 22/03/2021 19:41, Shawn McKinney wrote:
>>> On Mar 22, 2021, at 11:05 AM, Emmanuel Lécharny wrote:
>>>
>>> LDAP connections are quite stable. Again, this check is there to respect
>>> the commons-pool API contract. If the connection is dead, then doing this
>>> check will let the pool fetching another connection, which is good. OTOH,
>>> if you don't want to check connections while fetching one, then you are on
>>> your own (ie, deal with the consequences of a bad connection).
>> Again, I must disagree. Connections aren’t stable, subjected to env
>> conditions and we can never assume a connection is good.
>
> You *can* assume it's good, and react exceptionally if it's not. That the
> fastest way to deal with potential bad connections, if you want to avoid a
> check every time you pull a connection from the pool. (but see later for
> another option)
There are 150 locations in fortress core where an ldap connection is being
pulled from the pool. No way I want to revisit all of that code.
>
> Something in the API chain must do the job of testing and reconnect, every
> time it’s pulled from the pool.
>
> This is exactly what the testOnBorrow does ;-) But it's costly... (see later
> for another option)
>
>> Now, having said that, that’s exactly what I’m observing currently, with the
>> test on borrow flag turned off.
>> Let me explain the scenario:
>> 1. Start server
>> 2. Start client
>> This initializes a connection pool which creates and stores exactly 1
>> connection (min 1, max1 )
>> 3. Set breakpoint in client on pool.getConnection method
>> 4. Restart the server.
>> 5. Client performs ldap op which triggers the breakpoint on getConnection
>> 6. Server at this point still hasn’t any connections with the client. The
>> client ‘thinks’ it has connections in the pool, but these were broken on
>> step 4.
>> 7. Step over the getConnection which then triggers the commons pool to
>> execute:
>> ```
>> GenericObjectPool._factory.activateObject(latch.getPair().value)
>> ```
>> 8. A connection is made with the server, along with bind
>> ```
>> 6058e163 conn=1000 fd=14 ACCEPT from IP=127.0.0.1:35246 (IP=127.0.0.1:389)
>> [exec] 6058e163 conn=1000 op=0 BIND dn="cn=manager,dc=example,dc=com"
>> method=128
>> [exec] 6058e163 conn=1000 op=0 BIND dn="cn=manager,dc=example,dc=com"
>> mech=SIMPLE ssf=0
>> [exec] 6058e163 conn=1000 op=0 RESULT tag=97 err=0 duration=1.667ms
>> ```
>> 9. Client continues with its ldap op successfully and is never the wiser
>> that the connections were all forcibly closed on server restart.
>> This is EXACTLY what I want to see all of which is done without the test on
>> borrow eliminating the extraneous search on every connection retrieval.
>
> So that would mean we have some kind of 'retry' on connection operation: if
> it fails, then let's assume the connection is broken, and redo it with a
> fresh connection.
>
Yes, that’s what’s happening.
In the Commons pool lib, this is the block that gets executed:
``` this._factory.activateObject(latch.getPair().value);
if (this._testOnBorrow &&
!this._factory.validateObject(latch.getPair().value)) {
throw new Exception("ValidateObject failed");
}
```
In the first line above, activateObject, this code gets called, from our
AbstractPoolableLdapConnectionFactory class:
```
public void activateObject(LdapConnection connection) throws LdapException {
LOG.debug("Activating {}", connection);
if (!connection.isConnected() ||
!connection.isAuthenticated()) {
LOG.debug("rebind due to connection dropped on {}", connection);
this.connectionFactory.bindConnection(connection);
}
```
The code performs a rebind which renews the connection.
All of this with testOnBorrow being false!
So, I’m still scratching my head figuring why we need this secondary level that
is wasting a round trip to the server.
> The problem is that the pool is passive: it does not react to any connection
> event, like a connection closure. OTOH, when a connection is properly closed
> by the server (ie an NoticeOfDisconnect - aka NoD - is generated by the
> server), the connection should process it and die. Now we have an issue: the
> connection is just lying in the pool, not being used by any client, so there
> is a missing step: removing the connection from the pool in this very case.
> That can be something we can add to the current LDAP pool.
>
> Note that if the server does not send a NoD, you are screwed. There is no way
> to be sure that the connection is ok until you use it. OTOH, leveraging the
> setTestWhileIdle() could be a solution to partially workaround the issue.
Here you’ve lost me. I’m running the server in debug mode, inside a bash
shell, running in the foreground.
In my test I stop the server by ‘ctrl-c’ killing the shell.
The server is not reacting to this and sending a NoD.
—
Re: LdapConnectionPool.getConnection doing extraneous search?
> On Mar 22, 2021, at 2:21 PM, Stefan Seelmann wrote: > > Would a Java Dynamic Proxy help? > > It would implement the LdapConnection interface. > > When a method is invoked it borrows a connection from the pool (without > testing it), executes the method, returns the connection to the pool, > and returns the result. > > When the method execution fails with a specific error (connection error) > it invalidates the object in the pool, borrows a new one, and retries. Hi Stefan, thanks for weighing in. Potentially, yes. I’m still trying to determine if the connection pool behavior is going to be reliable with test on borrow turned off. From observations it does appear to, but I don’t understand ‘how’ yet and so remain skeptical. I’m definitely not satisfied with the test on borrow extra round trip and so we may have to go the dynamic proxy route. — Shawn - To unsubscribe, e-mail: api-unsubscr...@directory.apache.org For additional commands, e-mail: api-h...@directory.apache.org
Re: LdapConnectionPool.getConnection doing extraneous search?
On 22/03/2021 19:41, Shawn McKinney wrote: On Mar 22, 2021, at 11:05 AM, Emmanuel Lécharny wrote: LDAP connections are quite stable. Again, this check is there to respect the commons-pool API contract. If the connection is dead, then doing this check will let the pool fetching another connection, which is good. OTOH, if you don't want to check connections while fetching one, then you are on your own (ie, deal with the consequences of a bad connection). Again, I must disagree. Connections aren’t stable, subjected to env conditions and we can never assume a connection is good. You *can* assume it's good, and react exceptionally if it's not. That the fastest way to deal with potential bad connections, if you want to avoid a check every time you pull a connection from the pool. (but see later for another option) Something in the API chain must do the job of testing and reconnect, every time it’s pulled from the pool. This is exactly what the testOnBorrow does ;-) But it's costly... (see later for another option) Now, having said that, that’s exactly what I’m observing currently, with the test on borrow flag turned off. Let me explain the scenario: 1. Start server 2. Start client This initializes a connection pool which creates and stores exactly 1 connection (min 1, max1 ) 3. Set breakpoint in client on pool.getConnection method 4. Restart the server. 5. Client performs ldap op which triggers the breakpoint on getConnection 6. Server at this point still hasn’t any connections with the client. The client ‘thinks’ it has connections in the pool, but these were broken on step 4. 7. Step over the getConnection which then triggers the commons pool to execute: ``` GenericObjectPool._factory.activateObject(latch.getPair().value) ``` 8. A connection is made with the server, along with bind ``` 6058e163 conn=1000 fd=14 ACCEPT from IP=127.0.0.1:35246 (IP=127.0.0.1:389) [exec] 6058e163 conn=1000 op=0 BIND dn="cn=manager,dc=example,dc=com" method=128 [exec] 6058e163 conn=1000 op=0 BIND dn="cn=manager,dc=example,dc=com" mech=SIMPLE ssf=0 [exec] 6058e163 conn=1000 op=0 RESULT tag=97 err=0 duration=1.667ms ``` 9. Client continues with its ldap op successfully and is never the wiser that the connections were all forcibly closed on server restart. This is EXACTLY what I want to see all of which is done without the test on borrow eliminating the extraneous search on every connection retrieval. So that would mean we have some kind of 'retry' on connection operation: if it fails, then let's assume the connection is broken, and redo it with a fresh connection. The problem is that the pool is passive: it does not react to any connection event, like a connection closure. OTOH, when a connection is properly closed by the server (ie an NoticeOfDisconnect - aka NoD - is generated by the server), the connection should process it and die. Now we have an issue: the connection is just lying in the pool, not being used by any client, so there is a missing step: removing the connection from the pool in this very case. That can be something we can add to the current LDAP pool. Note that if the server does not send a NoD, you are screwed. There is no way to be sure that the connection is ok until you use it. OTOH, leveraging the setTestWhileIdle() could be a solution to partially workaround the issue. - To unsubscribe, e-mail: api-unsubscr...@directory.apache.org For additional commands, e-mail: api-h...@directory.apache.org - To unsubscribe, e-mail: api-unsubscr...@directory.apache.org For additional commands, e-mail: api-h...@directory.apache.org
