[Bug 65757] Async WriteListener#onWritePossible never called
https://bz.apache.org/bugzilla/show_bug.cgi?id=65757 Remy Maucherat changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #9 from Remy Maucherat --- The fix will be in 10.1.0-M9, 10.0.15, 9.0.57 and 8.5.74. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 65773] WINDOW_UPDATE NOT SENT
https://bz.apache.org/bugzilla/show_bug.cgi?id=65773 Mark Thomas changed: What|Removed |Added Resolution|--- |DUPLICATE Status|REOPENED|RESOLVED --- Comment #11 from Mark Thomas --- The original missing WINDOW_UPDATE issuer is a duplicate so I am restoring the resolution of this issue. That the client is now triggering the overhead protection is not a bug. If you want to explore why this is happening then the users list is the place to do so. If you enable debug logging: org.apache.coyote.http2.level = FINE in logging.properties then you'll see the overhead values changing as each frame is received and what is triggering the protection. *** This bug has been marked as a duplicate of bug 65179 *** -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 65179] HTTP2:WINDOW_UPDATE not sent when receiving http2 requests over unknown url
https://bz.apache.org/bugzilla/show_bug.cgi?id=65179 --- Comment #5 from Mark Thomas --- *** Bug 65773 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 65755] Users sporadically receive responses intended for other user's session
https://bz.apache.org/bugzilla/show_bug.cgi?id=65755
--- Comment #2 from dimitrios.psymar...@atos.net ---
Hi,
We have already checked if there are any possible "leaked" references to the
request/response/session objects outside of their normal scope, but this
doesn't seem to be the case.
Just to give you a better understanding of our situation, we have a Servlet
Filter which performs an automatic login based on a Cookie (by default this
Cookie expires after 30 days). After each automatic login this expiration date
is refreshed again in order to make it last for 30 days more. You can see a
simple representation of this implementation in the following snippet using
pseudocode for a better understanding:
**
public class AuthenticationCheckFilter implements Filter {
...
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain) throws IOException, ServletException {
...
// If (user is not logged)
// Read authentication cookie from "request" object.
// Perform automatic login using the cookie.
// Refresh the cookies expiration date in order to be valid
for
// 30 days more using the "response" object.
// End If
...
}
...
}
***
So if the user is not logged in (does not have a valid session at that point)
the authentication cookie (from the "request" object) is read and used in order
to perform an automatic login. After the successful completion of the
automatic login, the cookie's expiration date is refreshed (by using the
"response" object). The problem is that the response with the updated cookie
ends up to a different user session from the one that it was supposed to go.
So a different user will receive the SET-COOKIE header (with the updated
expiration date) that was intended for another user and he will be able to
eventually login to the other user's account.
In our understanding there is not something wrong with our logic here that
might end up mixing different user sessions since we don't keep any reference
on the request/response objects outside of the doFilter method. We just use the
request/response objects that are provided as parameters from the doFilter
method which are related to the current user session and not any other external
references on other sessions.
We are not sure if the previously mentioned errors are related to the mixing of
the different user sessions (or this could be a "silent" bug without traces)
but these are the only ones that are reported in the catalina logs.
Regarding you second question yes we are using Websockets and more specifically
we are using the atmosphere websocket framework (as it can be seen from the
second stack trace which seems to be related to WebSockets).
We have already upgraded our software to use the latest Tomcat version (8.5.73)
and so far the issue has not been reproduced yet (but keep in mind that this is
not easily reproduced). However since this is a really critical issue we
couldn't risk it by just waiting if the new version will fix this issue and
that's why we opened this ticket (we also couldn't find anything related in the
old reported/resolved issues).
In the past we were using version 8.0.49 (until it 8.0.X reached EOL) and we
later moved to version 8.5.43 but we never had such issues until the point we
upgraded to version 8.5.64. Also note that during these upgrades we didn't
perform any change to other stongly related (to this flow) libraries/frameworks
that could be "suspected" as problematic (websocket atmosphere framework,
Jersey) that's why we think that this is a Tomcat issue.
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: DataSourceUserDatabase questions
On Sat, Jan 1, 2022 at 10:07 PM Mark Thomas wrote: > > On 01/01/2022 20:05, Rémy Maucherat wrote: > > On Sat, Jan 1, 2022 at 8:20 PM Mark Thomas wrote: > >> > >> Hi all, > >> > >> Probably questions for Rémy but any input welcome. > >> > >> I'm looking at SpotBugs and it has a few warning for this class. My > >> questions are: > >> > >> 1. Should the methods the modify the database (createXXX, modifiedXXX, > >> removedXXX) use writeLock rather than readLock? > > > > This is copy/pasted from MemoryUserDatabase. I think this is probably > > as intended, this prevents a concurrent read, and the only actual > > write operation is persisting to the database. > > Thanks for the pointer. That helps. I think a little additional locking > is going to be required to keep everything consistent. > > >> 2. What sequence of actions between saves are meant to be supported? I'm > >> thinking about combinations of create/modify/delete? > > > > Any. Hopefully the user is a bit reasonable ;) It should work like the > > MemoryUserDatabase, modifications occur locally until the save > > operation which updates the database. The test case has scenarios, so > > if you find some more that you want to have supported they can be > > added there ... > > > > For example, if a user is created: > > - createdUsers.put and modifiedUsers.remove > > - the if you modify it, it won't do anything more since the user is > > already in createdUsers > > OK. Thanks. I thought I found a few problematic scenarios but I need to > think them through some more. Adding test cases is a good idea. Thanks for the sync improvements. As for the problematic scenarios, sequences like create/remove/create seem confusing when reading the code but do work. Rémy > > Mark > > - > 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: DataSourceUserDatabase questions
4 Jan 2022 13:03:57 Rémy Maucherat : Thanks for the sync improvements. As for the problematic scenarios, sequences like create/remove/create seem confusing when reading the code but do work. Agreed. I think I was getting confused by the possible ways to break it with concurrent requests. Once the locks were in place I couldn't see any way to break it. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
Thanks for the additional trace. That looks like a different issue. This
is next on my TODO list so hopefully I'll have something for you to test
later today.
Mark
On 03/01/2022 19:11, Rainer Jung wrote:
Unfortunately I can still reproduce a deadlock on 8.5. I saw the non-8.5
update a few minutes ago, so maybe more changes are coming. Just wanted
to let you know. jstack output attached. Reproduction based on
6285e750e2b640fa9c59c11c09a753a36a5396b8.
Am 03.01.2022 um 18:25 schrieb ma...@apache.org:
This is an automated email from the ASF dual-hosted git repository.
markt 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 6285e75 Improve fix to avoid deadlock reported on some systems
6285e75 is described below
commit 6285e750e2b640fa9c59c11c09a753a36a5396b8
Author: Mark Thomas
AuthorDate: Mon Jan 3 17:24:59 2022 +
Improve fix to avoid deadlock reported on some systems
https://markmail.org/message/ldpjfdwpkmqc7ved
---
java/org/apache/coyote/http2/Http2UpgradeHandler.java | 14
--
java/org/apache/coyote/http2/Stream.java | 18
++
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 1195997..a2c87ed 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -345,7 +345,7 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
// continue reading frames
Stream stream =
getStream(se.getStreamId(), false);
if (stream == null) {
- sendStreamReset(se);
+ sendStreamReset(null, se);
} else {
stream.close(se);
}
@@ -535,7 +535,7 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
}
- void sendStreamReset(StreamException se) throws IOException {
+ void sendStreamReset(StreamStateMachine state, StreamException
se) throws IOException {
if (log.isDebugEnabled()) {
log.debug(sm.getString("upgradeHandler.rst.debug",
connectionId,
@@ -554,7 +554,17 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
// Payload
ByteUtil.setFourBytes(rstFrame, 9, se.getError().getCode());
+ // Need to update state atomically with the sending of the RST
+ // frame else other threads currently working with this stream
+ // may see the state change and send a RST frame before the RST
+ // frame triggered by this thread. If that happens the client
+ // may see out of order RST frames which may hard to follow if
+ // the client is unaware the RST frames may be received out of
+ // order.
synchronized (socketWrapper) {
+ if (state != null) {
+ state.sendReset();
+ }
socketWrapper.write(true, rstFrame, 0, rstFrame.length);
socketWrapper.flush(true);
}
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index fffd240..a4d6484 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -614,14 +614,16 @@ class Stream extends AbstractNonZeroStream
implements HeaderEmitter {
log.debug(sm.getString("stream.reset.send",
getConnectionId(), getIdAsString(),
se.getError()));
}
- // Sync ensures that if the call to sendReset()
triggers resets
- // in other threads, that the RST frame associated
with this
- // thread is sent before the RST frames associated
with those
- // threads.
- synchronized (state) {
- state.sendReset();
- handler.sendStreamReset(se);
- }
+
+ // Need to update state atomically with the sending
of the RST
+ // frame else other threads currently working with
this stream
+ // may see the state change and send a RST frame
before the RST
+ // frame triggered by this thread. If that happens
the client
+ // may see out of order RST frames which may hard to
follow if
+ // the client is unaware the RST frames may be
received out of
+ // order.
+ handler.sendStreamReset(state, se);
+
cancelAllocationRequests();
if (inputBuffer != null) {
inputBuffer.swall
Re: [tomcat] branch 10.0.x updated: Update Eclipse JDT from 4.20 to 4.22
On 03/01/2022 21:19, Rémy Maucherat wrote: On Mon, Jan 3, 2022 at 12:32 PM Mark Thomas wrote: Sort of related, I am wondering about the build environment. We have seen an issue with downstream building with Java 11 and there are difficulties building 8.5.x particularly with a clean install of the latest publicly available Java 7. What about switching all versions to building with Java 11 and using the release compilation option to set the appropriate Java API to build against? Does that work for avoiding the ByteBuffer problems ? The docs I have read says it does but the cynic in me would want to explicitly test that first before committing the change to the repo. Assuming testing shows it does behave the way the docs says it does, what do you think? I don't think it would make a huge difference but it would simplify a few things and over time I think the time savings would make it worth while. It would certainly make it easier for anyone trying to build 8.5.x from source. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch 10.0.x updated: Update Eclipse JDT from 4.20 to 4.22
On Tue, Jan 4, 2022 at 4:09 PM Mark Thomas wrote: > > On 03/01/2022 21:19, Rémy Maucherat wrote: > > On Mon, Jan 3, 2022 at 12:32 PM Mark Thomas wrote: > > > > >> Sort of related, I am wondering about the build environment. We have > >> seen an issue with downstream building with Java 11 and there are > >> difficulties building 8.5.x particularly with a clean install of the > >> latest publicly available Java 7. What about switching all versions to > >> building with Java 11 and using the release compilation option to set > >> the appropriate Java API to build against? > > > > Does that work for avoiding the ByteBuffer problems ? > > The docs I have read says it does but the cynic in me would want to > explicitly test that first before committing the change to the repo. > > Assuming testing shows it does behave the way the docs says it does, > what do you think? I don't think it would make a huge difference but it > would simplify a few things and over time I think the time savings would > make it worth while. It would certainly make it easier for anyone trying > to build 8.5.x from source. Indeed, things are starting to be really problematic with 8.5 and Java 7 ... It can only become worse and 8.5 only becomes EOL once 11 is stable. This will take some time ... Another issue is after the change the testsuite will most likely never get run ever again with Java 8, so that could be a problem. So tentative +1 after testing. Rémy > Mark > > > - > 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
[tomcat] branch 8.5.x updated: Fix potential deadlock with concurrent new frames and close for stream
This is an automated email from the ASF dual-hosted git repository.
markt 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 1b2d056 Fix potential deadlock with concurrent new frames and close
for stream
1b2d056 is described below
commit 1b2d0560500958206a954013fa15ce96d12f83a7
Author: Mark Thomas
AuthorDate: Tue Jan 4 15:23:06 2022 +
Fix potential deadlock with concurrent new frames and close for stream
---
java/org/apache/coyote/http2/Stream.java | 11 +--
webapps/docs/changelog.xml | 4
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index a4d6484..b8aa5ce 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -1308,17 +1308,24 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
closed = true;
}
if (inBuffer != null) {
+int unreadByteCount = 0;
synchronized (inBuffer) {
-int unreadByteCount = inBuffer.position();
+unreadByteCount = inBuffer.position();
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.inputBuffer.swallowUnread",
Integer.valueOf(unreadByteCount)));
}
if (unreadByteCount > 0) {
inBuffer.position(0);
inBuffer.limit(inBuffer.limit() - unreadByteCount);
-handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
}
}
+// Do this outside of the sync because:
+// - it doesn't need to be inside the sync
+// - if inside the sync it can trigger a deadlock
+// https://markmail.org/message/vbglzkvj6wxlhh3p
+if (unreadByteCount > 0) {
+handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
+}
}
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 433fad9..0067fbd 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -189,6 +189,10 @@
65757: Missing initial IO listener notification on Servlet
container dispatch to another container thread. (remm)
+
+Avoid a potential deadlock during the concurrent processing of incoming
+HTTP/2 frames for a stream and that stream being reset. (markt)
+
-
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: Fix potential deadlock with concurrent new frames and close for stream
This is an automated email from the ASF dual-hosted git repository.
markt 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 39e4c4e Fix potential deadlock with concurrent new frames and close
for stream
39e4c4e is described below
commit 39e4c4e86c8552b3861a43955d6636ee5fbbbd72
Author: Mark Thomas
AuthorDate: Tue Jan 4 15:23:06 2022 +
Fix potential deadlock with concurrent new frames and close for stream
---
java/org/apache/coyote/http2/Stream.java | 11 +--
webapps/docs/changelog.xml | 4
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index ed8b90b..83a061c 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -1246,17 +1246,24 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
closed = true;
}
if (inBuffer != null) {
+int unreadByteCount = 0;
synchronized (inBuffer) {
-int unreadByteCount = inBuffer.position();
+unreadByteCount = inBuffer.position();
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.inputBuffer.swallowUnread",
Integer.valueOf(unreadByteCount)));
}
if (unreadByteCount > 0) {
inBuffer.position(0);
inBuffer.limit(inBuffer.limit() - unreadByteCount);
-handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
}
}
+// Do this outside of the sync because:
+// - it doesn't need to be inside the sync
+// - if inside the sync it can trigger a deadlock
+// https://markmail.org/message/vbglzkvj6wxlhh3p
+if (unreadByteCount > 0) {
+handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
+}
}
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index fdc8a4b..f64b3b6 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -285,6 +285,10 @@
on MacOS as it does on Linux and Windows when no trusted certificate
authorities are configured and reject all client certificates. (markt)
+
+Avoid a potential deadlock during the concurrent processing of incoming
+HTTP/2 frames for a stream and that stream being reset. (markt)
+
-
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: Fix potential deadlock with concurrent new frames and close for stream
This is an automated email from the ASF dual-hosted git repository.
markt 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 d77177d Fix potential deadlock with concurrent new frames and close
for stream
d77177d is described below
commit d77177dbe153078291605c1813cd9125dade4b41
Author: Mark Thomas
AuthorDate: Tue Jan 4 15:23:06 2022 +
Fix potential deadlock with concurrent new frames and close for stream
---
java/org/apache/coyote/http2/Stream.java | 11 +--
webapps/docs/changelog.xml | 4
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 1451a1b..ed5da75 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -1246,17 +1246,24 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
closed = true;
}
if (inBuffer != null) {
+int unreadByteCount = 0;
synchronized (inBuffer) {
-int unreadByteCount = inBuffer.position();
+unreadByteCount = inBuffer.position();
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.inputBuffer.swallowUnread",
Integer.valueOf(unreadByteCount)));
}
if (unreadByteCount > 0) {
inBuffer.position(0);
inBuffer.limit(inBuffer.limit() - unreadByteCount);
-handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
}
}
+// Do this outside of the sync because:
+// - it doesn't need to be inside the sync
+// - if inside the sync it can trigger a deadlock
+// https://markmail.org/message/vbglzkvj6wxlhh3p
+if (unreadByteCount > 0) {
+handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
+}
}
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index e0e3d32..d1befde 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -285,6 +285,10 @@
on MacOS as it does on Linux and Windows when no trusted certificate
authorities are configured and reject all client certificates. (markt)
+
+Avoid a potential deadlock during the concurrent processing of incoming
+HTTP/2 frames for a stream and that stream being reset. (markt)
+
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat] branch main updated: Fix potential deadlock with concurrent new frames and close for stream
This is an automated email from the ASF dual-hosted git repository.
markt 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 aa48dd9 Fix potential deadlock with concurrent new frames and close
for stream
aa48dd9 is described below
commit aa48dd90c654c778c224e4e4e827a4dce6a56305
Author: Mark Thomas
AuthorDate: Tue Jan 4 15:23:06 2022 +
Fix potential deadlock with concurrent new frames and close for stream
---
java/org/apache/coyote/http2/Stream.java | 11 +--
webapps/docs/changelog.xml | 4
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 8fee970..076226f 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -1251,17 +1251,24 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
closed = true;
}
if (inBuffer != null) {
+int unreadByteCount = 0;
synchronized (inBuffer) {
-int unreadByteCount = inBuffer.position();
+unreadByteCount = inBuffer.position();
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.inputBuffer.swallowUnread",
Integer.valueOf(unreadByteCount)));
}
if (unreadByteCount > 0) {
inBuffer.position(0);
inBuffer.limit(inBuffer.limit() - unreadByteCount);
-handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
}
}
+// Do this outside of the sync because:
+// - it doesn't need to be inside the sync
+// - if inside the sync it can trigger a deadlock
+// https://markmail.org/message/vbglzkvj6wxlhh3p
+if (unreadByteCount > 0) {
+handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
+}
}
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 7d95f41..fe1864e 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -323,6 +323,10 @@
on MacOS as it does on Linux and Windows when no trusted certificate
authorities are configured and reject all client certificates. (markt)
+
+Avoid a potential deadlock during the concurrent processing of incoming
+HTTP/2 frames for a stream and that stream being reset. (markt)
+
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
Hi Rainer,
I think I have this additional issue fixed. If you could test and let me
know how you get on that would be great.
Thanks,
Mark
On 04/01/2022 13:58, Mark Thomas wrote:
Thanks for the additional trace. That looks like a different issue. This
is next on my TODO list so hopefully I'll have something for you to test
later today.
Mark
On 03/01/2022 19:11, Rainer Jung wrote:
Unfortunately I can still reproduce a deadlock on 8.5. I saw the
non-8.5 update a few minutes ago, so maybe more changes are coming.
Just wanted to let you know. jstack output attached. Reproduction
based on 6285e750e2b640fa9c59c11c09a753a36a5396b8.
Am 03.01.2022 um 18:25 schrieb ma...@apache.org:
This is an automated email from the ASF dual-hosted git repository.
markt 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 6285e75 Improve fix to avoid deadlock reported on some
systems
6285e75 is described below
commit 6285e750e2b640fa9c59c11c09a753a36a5396b8
Author: Mark Thomas
AuthorDate: Mon Jan 3 17:24:59 2022 +
Improve fix to avoid deadlock reported on some systems
https://markmail.org/message/ldpjfdwpkmqc7ved
---
java/org/apache/coyote/http2/Http2UpgradeHandler.java | 14
--
java/org/apache/coyote/http2/Stream.java | 18
++
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 1195997..a2c87ed 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -345,7 +345,7 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
// continue reading frames
Stream stream =
getStream(se.getStreamId(), false);
if (stream == null) {
- sendStreamReset(se);
+ sendStreamReset(null, se);
} else {
stream.close(se);
}
@@ -535,7 +535,7 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
}
- void sendStreamReset(StreamException se) throws IOException {
+ void sendStreamReset(StreamStateMachine state, StreamException
se) throws IOException {
if (log.isDebugEnabled()) {
log.debug(sm.getString("upgradeHandler.rst.debug",
connectionId,
@@ -554,7 +554,17 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
// Payload
ByteUtil.setFourBytes(rstFrame, 9, se.getError().getCode());
+ // Need to update state atomically with the sending of the RST
+ // frame else other threads currently working with this stream
+ // may see the state change and send a RST frame before the RST
+ // frame triggered by this thread. If that happens the client
+ // may see out of order RST frames which may hard to follow if
+ // the client is unaware the RST frames may be received out of
+ // order.
synchronized (socketWrapper) {
+ if (state != null) {
+ state.sendReset();
+ }
socketWrapper.write(true, rstFrame, 0, rstFrame.length);
socketWrapper.flush(true);
}
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index fffd240..a4d6484 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -614,14 +614,16 @@ class Stream extends AbstractNonZeroStream
implements HeaderEmitter {
log.debug(sm.getString("stream.reset.send",
getConnectionId(), getIdAsString(),
se.getError()));
}
- // Sync ensures that if the call to sendReset()
triggers resets
- // in other threads, that the RST frame associated
with this
- // thread is sent before the RST frames associated
with those
- // threads.
- synchronized (state) {
- state.sendReset();
- handler.sendStreamReset(se);
- }
+
+ // Need to update state atomically with the sending
of the RST
+ // frame else other threads currently working with
this stream
+ // may see the state change and send a RST frame
before the RST
+ // frame triggered by this thread. If that happens
the client
+ // may see out of order RST frames which may hard to
follow if
+ // the client is unaware the RST frames may be
received out of
+ // orde
Re: [tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
Hi Mark, Am 04.01.2022 um 16:29 schrieb Mark Thomas: Hi Rainer, I think I have this additional issue fixed. If you could test and let me know how you get on that would be great. Great! LGTM, more than 200 iterations without deadlock. Before it neded about 3-10 iterations. Thanks a bunch, deadlocks are typically hard to fix. Best regards, Rainer - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] squarewav opened a new issue #24: Copyright and Derived Works
squarewav opened a new issue #24: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/24 Can you provide a statement in the README.md that describes the changes made by this tool? It is customary for software EULAs to include language like the following: `5. You may not reverse engineer the Software (such as by stepping through it with a debugger). You may not adapt, modify, translate or create derived works of the Software or accompanying documentation or any copy thereof, in whole or in part.` This raises an issue as to whether or not running this tool on a jar file creates a derived work that would violate a statement like the above. My feeling is that if the resulting jar is logically and functionally equivalent, then it is not a derived work, it is still the original work and therefore converting a library with this tool would not be a violation of an EULA with language like the above. However I do not know from glancing at the code, and I do not have authority to say, if the tool makes changes that are not logically and functionally equivalent. Can it be said that the only changes this tool makes are to package names? Or does it do more? Does it change method signatures, parameter types, or anything that might be construed as code? -- 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-jakartaee-migration] ebourg commented on issue #24: Copyright and Derived Works
ebourg commented on issue #24: URL: https://github.com/apache/tomcat-jakartaee-migration/issues/24#issuecomment-1005121746 > Can it be said that the only changes this tool makes are to package names? Yes > Does it change method signatures, parameter types Yes and yes, if the elements of the signature are in the Java EE namespace. Note that the README already states: > This migration tool performs all the necessary changes to migrate an application from Java EE 8 to Jakarta EE 9 by renaming each Java EE 8 package to its Jakarta EE 9 replacement. If you are concerned this might violate the licence of the application you are migrating the best is to ask to the vendor of the application if this is allowed, or if a Jakarta EE compatible version could be provided. -- 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
