Re: JNDIRealm not retrying connections on NamingException

2023-07-20 Thread Rémy Maucherat 
On Thu, Jul 20, 2023 at 12:30 PM Felix Schumacher
 wrote:
>
>
> Am 20.07.23 um 11:40 schrieb Rémy Maucherat:
>
> On Thu, Jul 20, 2023 at 11:11 AM Felix Schumacher
>  wrote:
>
> Hi all,
>
> at work, we have seen the following stacktrace without a retrying log message.
>
> javax.naming.NamingException: LDAP connection has been closed
> at com.sun.jndi.ldap.LdapRequest.getReplyBer(LdapRequest.java:133) 
> ~[?:1.8.0_342]
> at com.sun.jndi.ldap.Connection.readReply(Connection.java:469) 
> ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapClient.getSearchReply(LdapClient.java:638) 
> ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapClient.search(LdapClient.java:561) ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapCtx.doSearch(LdapCtx.java:2013) ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapCtx.searchAux(LdapCtx.java:1872) ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapCtx.c_search(LdapCtx.java:1797) ~[?:1.8.0_342]
> at 
> com.sun.jndi.toolkit.ctx.ComponentDirContext.p_search(ComponentDirContext.java:392)
>  ~[?:1.8.0_342]
> at 
> com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:358)
>  ~[?:1.8.0_342]
> at 
> com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:341)
>  ~[?:1.8.0_342]
> at 
> javax.naming.directory.InitialDirContext.search(InitialDirContext.java:267) 
> ~[?:1.8.0_342]
> at 
> org.apache.catalina.realm.JNDIRealm.getUserBySearch(JNDIRealm.java:1610) 
> ~[catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getUser(JNDIRealm.java:1447) 
> ~[catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getUser(JNDIRealm.java:1376) 
> ~[catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2348) 
> ~[catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2288) 
> [catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2253) 
> [catalina.jar:9.0.50.redhat-7]
>
> That happens, because we are catching CommunicationException and 
> ServiceUnavailableException in getPrincipal instead of the more general 
> NamingException.
>
> We had a similar issue in Bug 61313. To fix that bug we changed the catch 
> clause from CommunicationException to NamingException.
>
> I think we should change the code in getPrincipal to catch the more general 
> exception, too. Does anyone know, why we catched those specialized 
> NamingExceptions instead of the general one?
>
> I think the rationale was very simple: IO errors are always
> recoverable by closing and retrying the connection. Other errors are
> "". Now reading your exception it is "NamingException: LDAP
> connection has been closed", where it should have been
> "CommunicationException: LDAP connection has been closed". This is
> unfortunate.
> Your proposed change would mean everything is assumed to be
> recoverable which is not good, but unavoidable if everything is
> reported as a NamingException.
>
> I read your answer as, "ok, not nice, but let's do it".

Pretty much.

> And thanks for the explanation.
>
> But to add to the fun, I looked at the source code of a current OpenJDK 
> (https://github.com/openjdk/jdk/blob/94eb44b192ba421692549a178c386ea34164ea50/src/java.naming/share/classes/com/sun/jndi/ldap/LdapRequest.java#L115C26-L115C26).
>
> It looks like we now can expect to even get an IOException in case the LDAP 
> connection has been closed.

Ok, I read the code one level up in Connection.readReply, and the
IOException in LdapRequest gets wrapped as a CommunicationException.
So the workaround is not needed in Tomcat 11 (trunk) since it requires
Java 21. However, Java 17 (and 11 obviously) don't have the fix, so
the change is needed in 10.1 and 9.0.

Rémy

>
> Regards
>
>  Felix
>
> Rémy
>
> Regards
>
>  Felix
>
> PS. I will do a PR, if we agree on changing the catch clause.
>
> PPS. The code to catch the exception is the same in current tomcat JNDIRealm 
> classes, even if the line numbers changed a bit.
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: JNDIRealm not retrying connections on NamingException

2023-07-20 Thread Felix Schumacher 


Am 20.07.23 um 11:40 schrieb Rémy Maucherat:

On Thu, Jul 20, 2023 at 11:11 AM Felix Schumacher
  wrote:

Hi all,

at work, we have seen the following stacktrace without a retrying log message.

javax.naming.NamingException: LDAP connection has been closed
 at com.sun.jndi.ldap.LdapRequest.getReplyBer(LdapRequest.java:133) 
~[?:1.8.0_342]
 at com.sun.jndi.ldap.Connection.readReply(Connection.java:469) 
~[?:1.8.0_342]
 at com.sun.jndi.ldap.LdapClient.getSearchReply(LdapClient.java:638) 
~[?:1.8.0_342]
 at com.sun.jndi.ldap.LdapClient.search(LdapClient.java:561) ~[?:1.8.0_342]
 at com.sun.jndi.ldap.LdapCtx.doSearch(LdapCtx.java:2013) ~[?:1.8.0_342]
 at com.sun.jndi.ldap.LdapCtx.searchAux(LdapCtx.java:1872) ~[?:1.8.0_342]
 at com.sun.jndi.ldap.LdapCtx.c_search(LdapCtx.java:1797) ~[?:1.8.0_342]
 at 
com.sun.jndi.toolkit.ctx.ComponentDirContext.p_search(ComponentDirContext.java:392)
 ~[?:1.8.0_342]
 at 
com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:358)
 ~[?:1.8.0_342]
 at 
com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:341)
 ~[?:1.8.0_342]
 at 
javax.naming.directory.InitialDirContext.search(InitialDirContext.java:267) 
~[?:1.8.0_342]
 at 
org.apache.catalina.realm.JNDIRealm.getUserBySearch(JNDIRealm.java:1610) 
~[catalina.jar:9.0.50.redhat-7]
 at org.apache.catalina.realm.JNDIRealm.getUser(JNDIRealm.java:1447) 
~[catalina.jar:9.0.50.redhat-7]
 at org.apache.catalina.realm.JNDIRealm.getUser(JNDIRealm.java:1376) 
~[catalina.jar:9.0.50.redhat-7]
 at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2348) 
~[catalina.jar:9.0.50.redhat-7]
 at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2288) 
[catalina.jar:9.0.50.redhat-7]
 at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2253) 
[catalina.jar:9.0.50.redhat-7]

That happens, because we are catching CommunicationException and 
ServiceUnavailableException in getPrincipal instead of the more general 
NamingException.

We had a similar issue in Bug 61313. To fix that bug we changed the catch 
clause from CommunicationException to NamingException.

I think we should change the code in getPrincipal to catch the more general 
exception, too. Does anyone know, why we catched those specialized 
NamingExceptions instead of the general one?

I think the rationale was very simple: IO errors are always
recoverable by closing and retrying the connection. Other errors are
"". Now reading your exception it is "NamingException: LDAP
connection has been closed", where it should have been
"CommunicationException: LDAP connection has been closed". This is
unfortunate.
Your proposed change would mean everything is assumed to be
recoverable which is not good, but unavoidable if everything is
reported as a NamingException.


I read your answer as, "ok, not nice, but let's do it".

And thanks for the explanation.

But to add to the fun, I looked at the source code of a current OpenJDK 
(https://github.com/openjdk/jdk/blob/94eb44b192ba421692549a178c386ea34164ea50/src/java.naming/share/classes/com/sun/jndi/ldap/LdapRequest.java#L115C26-L115C26).


It looks like we now can expect to even get an IOException in case the 
LDAP connection has been closed.


Regards

 Felix



Rémy


Regards

  Felix

PS. I will do a PR, if we agree on changing the catch clause.

PPS. The code to catch the exception is the same in current tomcat JNDIRealm 
classes, even if the line numbers changed a bit.

-
To unsubscribe, e-mail:dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail:dev-h...@tomcat.apache.org



OpenPGP_0xEA6C3728EA91C4AF.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: JNDIRealm not retrying connections on NamingException

2023-07-20 Thread Rémy Maucherat 
On Thu, Jul 20, 2023 at 11:11 AM Felix Schumacher
 wrote:
>
> Hi all,
>
> at work, we have seen the following stacktrace without a retrying log message.
>
> javax.naming.NamingException: LDAP connection has been closed
> at com.sun.jndi.ldap.LdapRequest.getReplyBer(LdapRequest.java:133) 
> ~[?:1.8.0_342]
> at com.sun.jndi.ldap.Connection.readReply(Connection.java:469) 
> ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapClient.getSearchReply(LdapClient.java:638) 
> ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapClient.search(LdapClient.java:561) ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapCtx.doSearch(LdapCtx.java:2013) ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapCtx.searchAux(LdapCtx.java:1872) ~[?:1.8.0_342]
> at com.sun.jndi.ldap.LdapCtx.c_search(LdapCtx.java:1797) ~[?:1.8.0_342]
> at 
> com.sun.jndi.toolkit.ctx.ComponentDirContext.p_search(ComponentDirContext.java:392)
>  ~[?:1.8.0_342]
> at 
> com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:358)
>  ~[?:1.8.0_342]
> at 
> com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:341)
>  ~[?:1.8.0_342]
> at 
> javax.naming.directory.InitialDirContext.search(InitialDirContext.java:267) 
> ~[?:1.8.0_342]
> at 
> org.apache.catalina.realm.JNDIRealm.getUserBySearch(JNDIRealm.java:1610) 
> ~[catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getUser(JNDIRealm.java:1447) 
> ~[catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getUser(JNDIRealm.java:1376) 
> ~[catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2348) 
> ~[catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2288) 
> [catalina.jar:9.0.50.redhat-7]
> at org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2253) 
> [catalina.jar:9.0.50.redhat-7]
>
> That happens, because we are catching CommunicationException and 
> ServiceUnavailableException in getPrincipal instead of the more general 
> NamingException.
>
> We had a similar issue in Bug 61313. To fix that bug we changed the catch 
> clause from CommunicationException to NamingException.
>
> I think we should change the code in getPrincipal to catch the more general 
> exception, too. Does anyone know, why we catched those specialized 
> NamingExceptions instead of the general one?

I think the rationale was very simple: IO errors are always
recoverable by closing and retrying the connection. Other errors are
"". Now reading your exception it is "NamingException: LDAP
connection has been closed", where it should have been
"CommunicationException: LDAP connection has been closed". This is
unfortunate.
Your proposed change would mean everything is assumed to be
recoverable which is not good, but unavoidable if everything is
reported as a NamingException.

Rémy

>
> Regards
>
>  Felix
>
> PS. I will do a PR, if we agree on changing the catch clause.
>
> PPS. The code to catch the exception is the same in current tomcat JNDIRealm 
> classes, even if the line numbers changed a bit.

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

JNDIRealm not retrying connections on NamingException

2023-07-20 Thread Felix Schumacher 

Hi all,

at work, we have seen the following stacktrace without a retrying log 
message.


javax.naming.NamingException: LDAP connection has been closed
    at com.sun.jndi.ldap.LdapRequest.getReplyBer(LdapRequest.java:133) 
~[?:1.8.0_342]
    at com.sun.jndi.ldap.Connection.readReply(Connection.java:469) 
~[?:1.8.0_342]
    at com.sun.jndi.ldap.LdapClient.getSearchReply(LdapClient.java:638) 
~[?:1.8.0_342]
    at com.sun.jndi.ldap.LdapClient.search(LdapClient.java:561) 
~[?:1.8.0_342]
    at com.sun.jndi.ldap.LdapCtx.doSearch(LdapCtx.java:2013) 
~[?:1.8.0_342]
    at com.sun.jndi.ldap.LdapCtx.searchAux(LdapCtx.java:1872) 
~[?:1.8.0_342]
    at com.sun.jndi.ldap.LdapCtx.c_search(LdapCtx.java:1797) 
~[?:1.8.0_342]
    at 
com.sun.jndi.toolkit.ctx.ComponentDirContext.p_search(ComponentDirContext.java:392) 
~[?:1.8.0_342]
    at 
com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:358) 
~[?:1.8.0_342]
    at 
com.sun.jndi.toolkit.ctx.PartialCompositeDirContext.search(PartialCompositeDirContext.java:341) 
~[?:1.8.0_342]
    at 
javax.naming.directory.InitialDirContext.search(InitialDirContext.java:267) 
~[?:1.8.0_342]
    at 
org.apache.catalina.realm.JNDIRealm.getUserBySearch(JNDIRealm.java:1610) 
~[catalina.jar:9.0.50.redhat-7]
    at org.apache.catalina.realm.JNDIRealm.getUser(JNDIRealm.java:1447) 
~[catalina.jar:9.0.50.redhat-7]
    at org.apache.catalina.realm.JNDIRealm.getUser(JNDIRealm.java:1376) 
~[catalina.jar:9.0.50.redhat-7]
    at 
org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2348) 
~[catalina.jar:9.0.50.redhat-7]
    at 
org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2288) 
[catalina.jar:9.0.50.redhat-7]
    at 
org.apache.catalina.realm.JNDIRealm.getPrincipal(JNDIRealm.java:2253) 
[catalina.jar:9.0.50.redhat-7]


That happens, because we are catching CommunicationException and 
ServiceUnavailableException in getPrincipal instead of the more general 
NamingException.


We had a similar issue in Bug 61313. To fix that bug we changed the 
catch clause from CommunicationException to NamingException.


I think we should change the code in getPrincipal to catch the more 
general exception, too. Does anyone know, why we catched those 
specialized NamingExceptions instead of the general one?


Regards

 Felix

PS. I will do a PR, if we agree on changing the catch clause.

PPS. The code to catch the exception is the same in current tomcat 
JNDIRealm classes, even if the line numbers changed a bit.




OpenPGP_0xEA6C3728EA91C4AF.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature