[tomcat] branch main updated: Improve robustness
This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 67743cd Improve robustness
67743cd is described below
commit 67743cd39755886f5294f0ccd128b29fff956f0e
Author: remm
AuthorDate: Fri Oct 29 10:38:13 2021 +0200
Improve robustness
Make sure the connection is always unlocked.
---
java/org/apache/catalina/realm/JNDIRealm.java | 17 +++--
webapps/docs/changelog.xml| 5 +
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java
b/java/org/apache/catalina/realm/JNDIRealm.java
index 4f6cd07..a9dac4a 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -1279,7 +1279,7 @@ public class JNDIRealm extends RealmBase {
// Return the authenticated Principal (if any)
return principal;
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
@@ -2387,9 +2387,12 @@ public class JNDIRealm extends RealmBase {
// ... and have a password
return user.getPassword();
}
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
+// close the connection so we know it will be reopened.
+close(connection);
+closePooledConnections();
return null;
}
}
@@ -2459,7 +2462,7 @@ public class JNDIRealm extends RealmBase {
// Return the authenticated Principal (if any)
return principal;
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
@@ -2569,9 +2572,11 @@ public class JNDIRealm extends RealmBase {
*/
protected void release(JNDIConnection connection) {
if (connectionPool != null) {
-if (!connectionPool.push(connection)) {
-// Any connection that doesn't end back to the pool must be
closed
-close(connection);
+if (connection != null) {
+if (!connectionPool.push(connection)) {
+// Any connection that doesn't end back to the pool must
be closed
+close(connection);
+}
}
} else {
singleConnectionLock.unlock();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index ab48789..896f490 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -138,6 +138,11 @@
causes 'suspicious' (see the Servlet 6.0 specification) URIs to be
rejected with a 400 response. (markt)
+
+Improve robustness of JNDIRealm for exceptions occurring when getting
+the connection. Also add missing close when running into issues
+getting the passord of a user. (remm)
+
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat] branch 10.0.x updated: Improve robustness
This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch 10.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.0.x by this push:
new 0781316 Improve robustness
0781316 is described below
commit 0781316ed206b9814aea0e38e0f124f72a500d73
Author: remm
AuthorDate: Fri Oct 29 10:38:13 2021 +0200
Improve robustness
Make sure the connection is always unlocked.
---
java/org/apache/catalina/realm/JNDIRealm.java | 17 +++--
webapps/docs/changelog.xml| 9 +
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java
b/java/org/apache/catalina/realm/JNDIRealm.java
index 55f9f16..bbd1f59 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -1279,7 +1279,7 @@ public class JNDIRealm extends RealmBase {
// Return the authenticated Principal (if any)
return principal;
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
@@ -2387,9 +2387,12 @@ public class JNDIRealm extends RealmBase {
// ... and have a password
return user.getPassword();
}
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
+// close the connection so we know it will be reopened.
+close(connection);
+closePooledConnections();
return null;
}
}
@@ -2459,7 +2462,7 @@ public class JNDIRealm extends RealmBase {
// Return the authenticated Principal (if any)
return principal;
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
@@ -2566,9 +2569,11 @@ public class JNDIRealm extends RealmBase {
*/
protected void release(JNDIConnection connection) {
if (connectionPool != null) {
-if (!connectionPool.push(connection)) {
-// Any connection that doesn't end back to the pool must be
closed
-close(connection);
+if (connection != null) {
+if (!connectionPool.push(connection)) {
+// Any connection that doesn't end back to the pool must
be closed
+close(connection);
+}
}
} else {
singleConnectionLock.unlock();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index e22f5af..627a17b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -105,6 +105,15 @@
issues do not "pop up" wrt. others).
-->
+
+
+
+Improve robustness of JNDIRealm for exceptions occurring when getting
+the connection. Also add missing close when running into issues
+getting the passord of a user. (remm)
+
+
+
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat] branch 9.0.x updated: Improve robustness
This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 0df6494 Improve robustness
0df6494 is described below
commit 0df64948f249e18d01ec2ba69c74e87c70bc363c
Author: remm
AuthorDate: Fri Oct 29 10:38:13 2021 +0200
Improve robustness
Make sure the connection is always unlocked.
---
java/org/apache/catalina/realm/JNDIRealm.java | 17 +++--
webapps/docs/changelog.xml| 9 +
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java
b/java/org/apache/catalina/realm/JNDIRealm.java
index 6312b1f..80c829f 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -1279,7 +1279,7 @@ public class JNDIRealm extends RealmBase {
// Return the authenticated Principal (if any)
return principal;
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
@@ -2387,9 +2387,12 @@ public class JNDIRealm extends RealmBase {
// ... and have a password
return user.getPassword();
}
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
+// close the connection so we know it will be reopened.
+close(connection);
+closePooledConnections();
return null;
}
}
@@ -2460,7 +2463,7 @@ public class JNDIRealm extends RealmBase {
// Return the authenticated Principal (if any)
return principal;
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
@@ -2567,9 +2570,11 @@ public class JNDIRealm extends RealmBase {
*/
protected void release(JNDIConnection connection) {
if (connectionPool != null) {
-if (!connectionPool.push(connection)) {
-// Any connection that doesn't end back to the pool must be
closed
-close(connection);
+if (connection != null) {
+if (!connectionPool.push(connection)) {
+// Any connection that doesn't end back to the pool must
be closed
+close(connection);
+}
}
} else {
singleConnectionLock.unlock();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index eaf0e22..24fa7b0 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -105,6 +105,15 @@
issues do not "pop up" wrt. others).
-->
+
+
+
+Improve robustness of JNDIRealm for exceptions occurring when getting
+the connection. Also add missing close when running into issues
+getting the passord of a user. (remm)
+
+
+
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat] branch 8.5.x updated: Improve robustness
This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push:
new 240e93e Improve robustness
240e93e is described below
commit 240e93e59a4178af268468e78a7da0a493043788
Author: remm
AuthorDate: Fri Oct 29 10:38:13 2021 +0200
Improve robustness
Make sure the connection is always unlocked.
---
java/org/apache/catalina/realm/JNDIRealm.java | 17 +++--
webapps/docs/changelog.xml| 9 +
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java
b/java/org/apache/catalina/realm/JNDIRealm.java
index c16a444..498b4d2 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -1287,7 +1287,7 @@ public class JNDIRealm extends RealmBase {
// Return the authenticated Principal (if any)
return principal;
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
@@ -2402,9 +2402,12 @@ public class JNDIRealm extends RealmBase {
// ... and have a password
return user.getPassword();
}
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
+// close the connection so we know it will be reopened.
+close(connection);
+closePooledConnections();
return null;
}
}
@@ -2475,7 +2478,7 @@ public class JNDIRealm extends RealmBase {
// Return the authenticated Principal (if any)
return principal;
-} catch (NamingException e) {
+} catch (Exception e) {
// Log the problem for posterity
containerLog.error(sm.getString("jndiRealm.exception"), e);
@@ -2582,9 +2585,11 @@ public class JNDIRealm extends RealmBase {
*/
protected void release(JNDIConnection connection) {
if (connectionPool != null) {
-if (!connectionPool.push(connection)) {
-// Any connection that doesn't end back to the pool must be
closed
-close(connection);
+if (connection != null) {
+if (!connectionPool.push(connection)) {
+// Any connection that doesn't end back to the pool must
be closed
+close(connection);
+}
}
} else {
singleConnectionLock.unlock();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 0205463..8ce7802 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -105,6 +105,15 @@
issues do not "pop up" wrt. others).
-->
+
+
+
+Improve robustness of JNDIRealm for exceptions occurring when getting
+the connection. Also add missing close when running into issues
+getting the passord of a user. (remm)
+
+
+
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o opened a new pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o opened a new pull request #456: URL: https://github.com/apache/tomcat/pull/456 …to avoid JVM crashes This basically document cases to avoid issues like https://github.com/spring-projects/spring-boot/issues/28472 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
When did the path attribute in Context.xml go?
Hi, I am an occasional contributor to the Tomcat Eclipse Plugin [1]. One nice thing, that the plugin does is, that it creates a Context.xml for you, so that you can have the webapp in-place while developing. Right now, that Context.xml would look like this: Well, with recent Tomcat versions, that is no longer correct, because the path attribute is rejected. A message appears in the log file: The path attribute with value [/iaas] in deployment descriptor [C:\opt\apache-tomcat-9.0.54\conf\Catalina\localhost\iaas.xml] has been ignored Not a real problem, but something, that I would like to see changed. Question: Can you tell me (as exactly as possible), which Tomcat versions want that attribute? Right now, my plan would be: Keep the attribute for Tomcat < 9. (It is required.) Keep the attribute for Tomcat 9 (It may be required. If not, no harm done.) Remove the attribute for Tomcat 10. Thanks, Jochen 1: https://github.com/tomcatplugin/tomcatplugin -- Philosophy is useless, theology is worse. (Industrial Desease, Dire Straits) - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: When did the path attribute in Context.xml go?
On 29/10/2021 10:22, Jochen Wiedmann wrote: Hi, I am an occasional contributor to the Tomcat Eclipse Plugin [1]. One nice thing, that the plugin does is, that it creates a Context.xml for you, so that you can have the webapp in-place while developing. Right now, that Context.xml would look like this: Well, with recent Tomcat versions, that is no longer correct, because the path attribute is rejected. A message appears in the log file: The path attribute with value [/iaas] in deployment descriptor [C:\opt\apache-tomcat-9.0.54\conf\Catalina\localhost\iaas.xml] has been ignored Not a real problem, but something, that I would like to see changed. Question: Can you tell me (as exactly as possible), which Tomcat versions want that attribute? Have you tried reading the documentation? Going back to at least Tomcat 5.5 (2004) the documentation for the path attribute states: "The value of this field must not be set except when statically defining a Context in server.xml, as it will be inferred from the filenames used for either the .xml context file or the docBase." Right now, my plan would be: Keep the attribute for Tomcat < 9. (It is required.) Nope. It is not required for any current (and at least the 3 previous EOL'd versions) and will be ignored. Keep the attribute for Tomcat 9 (It may be required. If not, no harm done.) Unnecessary - it will be ignored. Remove the attribute for Tomcat 10. This is the correct approach for all current versions (and the EOL'd versions back to at least 5.5.x) Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] wilkinsona commented on a change in pull request #456: Document conditions under which the AprLifecycleListener can be used …
wilkinsona commented on a change in pull request #456:
URL: https://github.com/apache/tomcat/pull/456#discussion_r739128184
##
File path: java/org/apache/catalina/core/AprLifecycleListener.java
##
@@ -38,6 +38,13 @@
/**
* Implementation of LifecycleListener that will init and
* and destroy APR.
+ *
+ * Note: This listener must only be used within a {@code
Server}
+ * element to manage APR/OpenSSL init and destroy JVM-wide. If you are running
+ * Tomcat in an embedded fashion and have more than one Tomcat instance per
JVM,
+ * this listener must not be added to the {@code Server} instance, but
Review comment:
Maybe this is too subtle for Tomcat's javadoc, but this isn't strictly
true. You can safely add the listener to a `Server` (or even a `Context` if you
know there will only be one) as long as that `Server` is started first and
stopped last. In the absence of an API that's designed to manage the lifecycle
of APR outside of Tomcat's `Lifecycle` and `LifecycleEvent`, I suspect that's
what we'll do in Spring Boot if the Tomcat team decides not to make
`AprLifecycleListener` more robust.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: When did the path attribute in Context.xml go?
Hi, Mark, yes, I *did* try to read the documentation. Problem is, if you don't know exactly, what you are looking for, things are hard to found. (The word "path" is used frequently, so Google won't help.) that said: Thanks for your reply, which provided all the information, that I need. Jochen - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o commented on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954648720 > > > This feels like a step forwards, but ideally one that will be reverted in the future as AprLifecycleListener is made more robust. It's difficult to gauge the need for this change prior to a decision being made about that increased robustness. The only robustness I see is that the client code adds to `Server` only as intended, because connectors are initiated *before* a context and destroyed *after* a context. Context level will still remain wrong. Plus reference counting if there is more than one `Sever` in the JVM. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on a change in pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o commented on a change in pull request #456:
URL: https://github.com/apache/tomcat/pull/456#discussion_r739135782
##
File path: java/org/apache/catalina/core/AprLifecycleListener.java
##
@@ -38,6 +38,13 @@
/**
* Implementation of LifecycleListener that will init and
* and destroy APR.
+ *
+ * Note: This listener must only be used within a {@code
Server}
+ * element to manage APR/OpenSSL init and destroy JVM-wide. If you are running
+ * Tomcat in an embedded fashion and have more than one Tomcat instance per
JVM,
+ * this listener must not be added to the {@code Server} instance, but
Review comment:
I almost agree with your statement. What would you change in the
documentation if the code remains the same?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on a change in pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o commented on a change in pull request #456:
URL: https://github.com/apache/tomcat/pull/456#discussion_r739135782
##
File path: java/org/apache/catalina/core/AprLifecycleListener.java
##
@@ -38,6 +38,13 @@
/**
* Implementation of LifecycleListener that will init and
* and destroy APR.
+ *
+ * Note: This listener must only be used within a {@code
Server}
+ * element to manage APR/OpenSSL init and destroy JVM-wide. If you are running
+ * Tomcat in an embedded fashion and have more than one Tomcat instance per
JVM,
+ * this listener must not be added to the {@code Server} instance, but
Review comment:
I almost agree with your statement. What would you change in the
documentation if the code remains the same? At least the current contract is
fully defined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o edited a comment on pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o edited a comment on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954648720 > > > This feels like a step forwards, but ideally one that will be reverted in the future as AprLifecycleListener is made more robust. It's difficult to gauge the need for this change prior to a decision being made about that increased robustness. The only robustness I see is that the client code adds to `Server` only as intended, because connectors are initiated *before* a context and destroyed *after* a context. Context level will still remain wrong. Plus reference counting if there is more than one `Server` in the JVM. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: When did the path attribute in Context.xml go?
On 29/10/2021 11:56, Jochen Wiedmann wrote: Hi, Mark, yes, I *did* try to read the documentation. Problem is, if you don't know exactly, what you are looking for, things are hard to found. (The word "path" is used frequently, so Google won't help.) If you are looking for an explanation of a Tomcat configuration attribute then: http://tomcat.apache.org/ Pick the correct documentation link for your version from the LHS menu. e.g.: http://tomcat.apache.org/tomcat-9.0-doc/index.html Then under reference select configuration: http://tomcat.apache.org/tomcat-9.0-doc/config/index.html Then select the appropriate component - Context in this case: http://tomcat.apache.org/tomcat-9.0-doc/config/context.html Jump to the attributes section: http://tomcat.apache.org/tomcat-9.0-doc/config/context.html#Attributes And the attributes will be listed in alphabetical order. Depending on the component, there will be some combination of: - Common Attributes (all implementations of the component support these - Standard Implementation (additional attributes supported by the default configuration) - Additional implementations (additional attributes supported by those implementations) Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
ChristopherSchultz commented on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954890217 Something is wrong with the premise, here. I read through [Spring-28472](https://github.com/spring-projects/spring-boot/issues/28472) and looked at the code for `AprLifecycleListener`. `AprLifecycleListener` should not be shutting-down APR twice. In `AprLifecycleListener.lifecycleEvent`, the `Lifecycle.AFTER_DESTROY` handler uses a static lock for cross-thread synchronization and only calls `terminateAPR` if `AprStatus.isAprAvailable` returns `true`. Notably, `terminateAPR` sets `AprStatus.isAprAvailable` to `false`. I can see a theoretical threading problem because `AprStatus.isAprAvailable` and `AprStatus.setAprAvailable` are not synchronized, but the static class members being set are declared `volatile` and should require threads to reload their values appropriately. Are you sure it's the shutdown that's causing the failure? I think I'd rather fix a bug that seems to be hiding, here, rather than document what can and cannot be done (in a way that is very difficult to understand, I have to admit). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o commented on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954898919 > > > Something is wrong with the premise, here. I read through [Spring-28472](https://github.com/spring-projects/spring-boot/issues/28472) and looked at the code for `AprLifecycleListener`. `AprLifecycleListener` should not be shutting-down APR twice. In `AprLifecycleListener.lifecycleEvent`, the `Lifecycle.AFTER_DESTROY` handler uses a static lock for cross-thread synchronization and only calls `terminateAPR` if `AprStatus.isAprAvailable` returns `true`. Notably, `terminateAPR` sets `AprStatus.isAprAvailable` to `false`. > > I can see a theoretical threading problem because `AprStatus.isAprAvailable` and `AprStatus.setAprAvailable` are not synchronized, but the static class members being set are declared `volatile` and should require threads to reload their values appropriately. > > Are you sure it's the shutdown that's causing the failure? That's not the failure. Please read my description again. The first Tomcat instance starts `AprLifecycleListener`. The scond skips it. Shutdown does reverse order, so the second Tomcat now shuts down APR globally, releasing the global structures. The first Tomcat is now shutting down, its `AprConnector` tries to use a sub APR pool/object from a pool which has already been released. See * https://github.com/apache/tomcat-native/blob/c17d3e0e0a594604ff4f30065360aacc688cfb50/native/src/jnilib.c#L243-L262 * https://github.com/apache/tomcat/blob/ae4ee893f99f226fe5fdc8e933313e2e759d0733/java/org/apache/tomcat/jni/Library.java#L102-L109 * https://apr.apache.org/docs/apr/1.6/group__apr__library.html#ga4a91a6b9ff457ead13e670950127761a -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] wilkinsona commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
wilkinsona commented on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954908926 I agree, @ChristopherSchultz. IMO, the second `AprLifecycleListener` should not be shutting down APR if it did nothing when asked to initialize it. The ordering in Spring Boot is the following: 1. Start main Tomcat instance 2. Start management Tomcat instance 3. Stop management Tomcat instance 4. Stop main Tomcat instance From an APR perspective, the following happens at the four steps above: 1. APR is initialized in response to `BEFORE_INIT_EVENT` 2. APR is already initialized so handling of `BEFORE_INIT_EVENT` is essentially a no-op 3. APR is terminated in `AFTER_DESTROY_EVENT` The JVM crashes before we get to step 4 as APR is been ripped out from underneath the main Tomcat instance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o commented on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954912903 @wilkinsona Correct. The connector on main Tomcat is still active while APR is already gone. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
ChristopherSchultz commented on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954934999 > [...] so the second Tomcat now shuts down APR globally, releasing the global structures. The first Tomcat is now shutting down, its `AprConnector` tries to use a sub APR pool/object from a pool which has already been released. There is no `AprConnector`. Where is the Java call which eventually goes down into native to cause this problem? As the AprLifecycleListener is stopping, it should not try to use any APR pool. Perhaps the other connector is still trying to actually use pool objects in some other way. If that's the case, the problem isn't with double-shutdown. It's with shutdown-while-trying-to-still-do-stuff. See > * https://github.com/apache/tomcat-native/blob/c17d3e0e0a594604ff4f30065360aacc688cfb50/native/src/jnilib.c#L243-L262 Notably, even this native method prevents crashes due to double-shutdown. The predicate around the whole thing prevents it from being called twice. So can we get a Java stack trace for where this is crashing? It should be simple to check to see if APR has been killed at some point and at least not crash the JVM. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz edited a comment on pull request #456: Document conditions under which the AprLifecycleListener can be used …
ChristopherSchultz edited a comment on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954934999 > [...] so the second Tomcat now shuts down APR globally, releasing the global structures. The first Tomcat is now shutting down, its `AprConnector` tries to use a sub APR pool/object from a pool which has already been released. There is no `AprConnector`. Where is the Java call which eventually goes down into native to cause this problem? As the AprLifecycleListener is stopping, it should not try to use any APR pool. Perhaps the other connector is still trying to actually use pool objects in some other way. If that's the case, the problem isn't with double-shutdown. It's with shutdown-while-trying-to-still-do-stuff. > See > * https://github.com/apache/tomcat-native/blob/c17d3e0e0a594604ff4f30065360aacc688cfb50/native/src/jnilib.c#L243-L262 Notably, even this native method prevents crashes due to double-shutdown. The predicate around the whole thing prevents it from being called twice. So can we get a Java stack trace for where this is crashing? It should be simple to check to see if APR has been killed at some point and at least not crash the JVM. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o commented on pull request #456: URL: https://github.com/apache/tomcat/pull/456#issuecomment-954947125 > > > > [...] so the second Tomcat now shuts down APR globally, releasing the global structures. The first Tomcat is now shutting down, its `AprConnector` tries to use a sub APR pool/object from a pool which has already been released. > > There is no `AprConnector`. I actually meant the connector using the APR Protocol with the APR endpoint. Where is the Java call which eventually goes down into native to cause this problem? As the AprLifecycleListener is stopping, it should not try to use any APR pool. Perhaps the other connector is still trying to actually use pool objects in some other way. If that's the case, the problem isn't with double-shutdown. It's with shutdown-while-trying-to-still-do-stuff. > > So can we get a Java stack trace for where this is crashing? It should be simple to check to see if APR has been killed at some point and at least not crash the JVM. How does this actually matter? If APR has been globally terminated any subsequent operation to any APR object is deemed to fail and that this what happens here. The problem is just init and destroy and wrong points in time. That's it. ``` Stack: [0x7fffd000,0x7fffdcdcc000], sp=0x7fffdcdcae70, free space=1019k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) C [libapr-1.so.0+0x336fa] signed char+0x1a C [libtcnative-1.so.0.2.24+0x19a06] Java_org_apache_tomcat_jni_Address_get+0x36 j org.apache.tomcat.jni.Address.get(IJ)J+0 j org.apache.tomcat.util.net.AprEndpoint.getLocalAddress()Ljava/net/InetSocketAddress;+15 j org.apache.tomcat.util.net.AbstractEndpoint.unlockAccept()V+26 j org.apache.tomcat.util.net.AbstractEndpoint.closeServerSocketGraceful()V+23 j org.apache.coyote.AbstractProtocol.closeServerSocketGraceful()V+4 j org.apache.catalina.core.StandardService.stopInternal()V+35 j org.apache.catalina.util.LifecycleBase.stop()V+214 j org.apache.catalina.core.StandardServer.stopInternal()V+82 j org.apache.catalina.util.LifecycleBase.stop()V+214 j org.apache.catalina.startup.Tomcat.stop()V+9 j org.springframework.boot.web.embedded.tomcat.TomcatWebServer.stopTomcat()V+29 j org.springframework.boot.web.embedded.tomcat.TomcatWebServer.stop()V+32 j org.springframework.boot.web.servlet.context.WebServerStartStopLifecycle.stop()V+4 j org.springframework.context.SmartLifecycle.stop(Ljava/lang/Runnable;)V+1 ``` This is from the main Tomcat instance and shutdown time. The management one is already gone. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
michael-o commented on pull request #456:
URL: https://github.com/apache/tomcat/pull/456#issuecomment-954959277
@ChristopherSchultz Here is an extremely primitive test case:
```java
public class TomcatEmbeddedTest {
static class ShutdownHook2 extends Thread {
Tomcat tomcat;
Tomcat tomcat2;
public void run() {
try {
Thread.sleep(5000L);
} catch (InterruptedException e1) {
e1.printStackTrace();
}
try {
tomcat2.stop();
tomcat2.destroy();
tomcat.stop();
tomcat.destroy();
} catch (LifecycleException e) {
e.printStackTrace();
}
}
}
public static void main(String[] args) throws LifecycleException {
Tomcat tomcat = new Tomcat();
tomcat.setPort(1);
AprLifecycleListener listener = new AprLifecycleListener();
listener.setUseAprConnector(true);
listener.setUseOpenSSL(true);
tomcat.getServer().addLifecycleListener(listener);
tomcat.getConnector();
tomcat.init();
Tomcat tomcat2 = new Tomcat();
tomcat2.setPort(2);
AprLifecycleListener listener2 = new AprLifecycleListener();
listener2.setUseAprConnector(true);
listener2.setUseOpenSSL(true);
tomcat2.getServer().addLifecycleListener(listener2);
tomcat2.getConnector();
tomcat2.init();
ShutdownHook2 sh2 = new ShutdownHook2();
sh2.tomcat = tomcat;
sh2.tomcat2 = tomcat2;
sh2.start();
tomcat.start();
tomcat2.start();
}
}
```
Output:
```
Okt 29, 2021 8:26:01 PM org.apache.catalina.core.AprLifecycleListener
lifecycleEvent
INFORMATION: Loaded Apache Tomcat Native library [1.2.31] using APR version
[1.7.0].
Okt 29, 2021 8:26:01 PM org.apache.catalina.core.AprLifecycleListener
lifecycleEvent
INFORMATION: APR capabilities: IPv6 [true], sendfile [true], accept filters
[false], random [true], UDS [true].
Okt 29, 2021 8:26:01 PM org.apache.catalina.core.AprLifecycleListener
lifecycleEvent
INFORMATION: APR/OpenSSL configuration: useAprConnector [true], useOpenSSL
[true]
Okt 29, 2021 8:26:01 PM org.apache.catalina.core.AprLifecycleListener
initializeSSL
INFORMATION: OpenSSL successfully initialized [OpenSSL 1.1.1l 24 Aug 2021]
Okt 29, 2021 8:26:01 PM org.apache.coyote.AbstractProtocol init
INFORMATION: Initializing ProtocolHandler ["http-apr-1"]
Okt 29, 2021 8:26:01 PM org.apache.coyote.AbstractProtocol init
INFORMATION: Initializing ProtocolHandler ["http-apr-2"]
Okt 29, 2021 8:26:01 PM org.apache.catalina.core.StandardService
startInternal
INFORMATION: Starting service [Tomcat]
Okt 29, 2021 8:26:01 PM org.apache.coyote.AbstractProtocol start
INFORMATION: Starting ProtocolHandler ["http-apr-1"]
Okt 29, 2021 8:26:01 PM org.apache.catalina.core.StandardService
startInternal
INFORMATION: Starting service [Tomcat]
Okt 29, 2021 8:26:01 PM org.apache.coyote.AbstractProtocol start
INFORMATION: Starting ProtocolHandler ["http-apr-2"]
Okt 29, 2021 8:26:06 PM org.apache.coyote.AbstractProtocol pause
INFORMATION: Pausing ProtocolHandler ["http-apr-2"]
Okt 29, 2021 8:26:06 PM org.apache.catalina.core.StandardService stopInternal
INFORMATION: Stopping service [Tomcat]
Okt 29, 2021 8:26:06 PM org.apache.coyote.AbstractProtocol stop
INFORMATION: Stopping ProtocolHandler ["http-apr-2"]
Okt 29, 2021 8:26:06 PM org.apache.coyote.AbstractProtocol destroy
INFORMATION: Destroying ProtocolHandler ["http-apr-2"]
Okt 29, 2021 8:26:06 PM org.apache.coyote.AbstractProtocol pause
INFORMATION: Pausing ProtocolHandler ["http-apr-1"]
#
# A fatal error has been detected by the Java Runtime Environment:
#
# EXCEPTION_ACCESS_VIOLATION (0xc005) at pc=0x0001800011dd,
pid=19692, tid=0x4138
#
# JRE version: OpenJDK Runtime Environment (Zulu 8.52.0.24-SA-win64)
(8.0_282-b08) (build 1.8.0_282-b08)
# Java VM: OpenJDK 64-Bit Server VM (25.282-b08 mixed mode windows-amd64
compressed oops)
# Problematic frame:
# Okt 29, 2021 8:26:06 PM org.apache.tomcat.util.net.Acceptor run
SCHWERWIEGEND: Socket accept failed
org.apache.tomcat.jni.Error: 730004: Ein Blockierungsvorgang wurde durch
einen Aufruf von WSACancelBlockingCall unterbrochen.
at org.apache.tomcat.jni
[GitHub] [tomcat] ChristopherSchultz commented on pull request #456: Document conditions under which the AprLifecycleListener can be used …
ChristopherSchultz commented on pull request #456:
URL: https://github.com/apache/tomcat/pull/456#issuecomment-955018111
Your test-case looks even more complicated than necessary: just initialize
two of them then deinitialize them. No shutdown hook necessary, right?
Okay, so it's not crashing in the `AprLifecycleListener`'s shutdown, which
is what it sounded like you were reporting.
Certainly, anything can crash at any time after the APR global pools have
been shut-down. We could put guards around those things. Something like this at
the top of each of the calls which require APR:
```
#DEFINE CHECK_APR_INITIALIZED(ENV) { \
if(!tcn_global_pool) { \
tcn_ThrowAPRException((ENV), APR_EINIT); \
} \
} \
TCN_IMPLEMENT_CALL(jstring, Address, getnameinfo)(TCN_STDARGS,
jlong sa, jint flags)
{
apr_sockaddr_t *s = J2P(sa, apr_sockaddr_t *);
char *hostname;
UNREFERENCED(o);
CHECK_APR_INITIALIZED(e); /* <- This macro invocation is new */
if (apr_getnameinfo(&hostname, s, (apr_int32_t)flags) == APR_SUCCESS)
return AJP_TO_JSTRING(hostname);
else
return NULL;
}
```
It would be much cleaner to implement this at the Java level, but as your
test-case demonstrates, it's always possible for APR to disappear during the
execution of one of the native methods.
I'm kind of curious as to exactly where in `Socket.accept()` this particular
failure occurred. We use APR pools for things like string values, even to throw
exceptions. So it's hard to avoid using APR anywhere. One could argue that APR
pools aren't necessary for throwing exceptions, but re-writing tcnative at this
point to remove APR-type things is unlikely. Perhaps incrementally. But Rémy's
recent work with Project Panama looks like we might be able to dump tcnative in
the reasonably-near future.
Anyway, back to fixing this kind of thing:
1. I think it's worth mentioning the dangers of multiple
AprLifecycleListeners in the Tomcat documentation. I don't think the PR as
submitted goes far enough. I think it's even misleading at first (e.g. only use
`AprLifecycleListener` on a `Server` element) because there are many ways to
use the `AprLifecycleListener` in ways that can cause the JVM to crash. I think
this documentation should be changed in the Javadoc but also in the
documentation for the listener in general, here
http://tomcat.apache.org/tomcat-10.0-doc/apr.html#APR_Lifecycle_Listener_Configuration
and here
http://tomcat.apache.org/tomcat-10.0-doc/config/listeners.html#APR_Lifecycle_Listener_-_org.apache.catalina.core.AprLifecycleListener.
(Note that the second of these two already states that the listener should
only be used on `` components.)
1. I think it's also reasonable to try to protect the JVM against this kind
of failure. Anyone starting two Tomcats in a single JVM is going to have
exactly the same problem. Yes, it's possible to have the "container" (e.g.
Spring in this case) manage the whole, um, _lifecycle_ of the
`AprLifecycleListener` but it's much more natural to use it "inside" Tomcat and
allow Tomcat to manage that process.
I like the idea of reference-counting, especially because the number of
times the `AprLifecycleListener` is initialized and de-initialized in a given
JVM should be relatively low. It's a small amount of code to manage, provides a
great amount of protection (JVM crashes should really never be tolerated), and
the performance impact is irrelevant.
Would you care to prepare a reference-counting patch for
`AprLifecycleListener` either as a part of this PR, or as a separate one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
