You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2022/01/03 17:25:34 UTC
[tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
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 <ma...@apache.org>
AuthorDate: Mon Jan 3 17:24:59 2022 +0000
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.swallowUnread();
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
Posted by Mark Thomas <ma...@apache.org>.
On 04/01/2022 18:24, Rainer Jung wrote:
> 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.
Excellent.
> Thanks a bunch, deadlocks are typically hard to fix.
Fortunately, these weren't too tricky to fix. That you were able to
provide the thread dumps made it a lot easier. Thanks for all your help.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
Posted by Rainer Jung <ra...@kippdata.de>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
Posted by Mark Thomas <ma...@apache.org>.
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 markt@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 <ma...@apache.org>
>>> AuthorDate: Mon Jan 3 17:24:59 2022 +0000
>>>
>>> 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.swallowUnread();
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
Posted by Mark Thomas <ma...@homeinbox.net>.
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 markt@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 <ma...@apache.org>
>> AuthorDate: Mon Jan 3 17:24:59 2022 +0000
>>
>> 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.swallowUnread();
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Improve fix to avoid deadlock reported on some systems
Posted by Rainer Jung <ra...@kippdata.de>.
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 markt@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 <ma...@apache.org>
> AuthorDate: Mon Jan 3 17:24:59 2022 +0000
>
> 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.swallowUnread();