UNDERTOW-2424: Fix the request race produced by #1495 #1645
Conversation
The failure was the result of disagreement over the owner of a connection, after an initial thread ends an exchange and releases the connection, but continues to operate upon it with a premature `flush()` from an illegal thread (per undertow thread model). The HttpServerExchange uses a `WriteDispatchChannel` in order to avoid interfering with the underlying connection after the exchange is completed, however the `WriteDispatchChannel` is an outer layer, around all response-wrappers. Response wrappers themselves may still interact with the underlying connections even after the `HttpReadListener` state has been reset to allow another request to be parsed. We must wrap the lower level streams with a similar function to prevent this spillover. In practice, the failure I've encountered and reproduced was the result of `DeflatingStreamSinkConduit` invoking `next.terminateWrites()` followed by `next.flush()`. The `terminateWrites()` invocation causes the delegate `ChunkedStreamSinkConduit` to flush within `terminateWrites` which triggers the end of the exchange. Then, the following `flush()` in `DeflatingStreamSinkConduit` applies to another request entirely. This can be reproduced quickly using the reproducer branch (undertow-io#1495) with a slight code modification, adding a `sleep(100)` thusly: ```diff if (performFlushIfRequired()) { state |= NEXT_SHUTDOWN; freeBuffer(); next.terminateWrites(); + try { + Thread.sleep(100); + } catch (InterruptedException e) {} return next.flush(); } else { ```
| @@ -1397,7 +1397,7 @@ public StreamSinkChannel getResponseChannel() { | |||
| final WrapperStreamSinkConduitFactory factory = new WrapperStreamSinkConduitFactory(wrappers, responseWrapperCount, this, sinkChannel.getConduit()); | |||
| sinkChannel.setConduit(factory.create()); | |||
| } else { | |||
| sinkChannel.setConduit(connection.getSinkConduit(this, sinkChannel.getConduit())); | |||
| sinkChannel.setConduit(new ExchangeLifecycleDetachableStreamSinkConduit(connection.getSinkConduit(this, sinkChannel.getConduit()), this)); | |||
There was a problem hiding this comment.
In this case there are no wrappers, so we don't necessarily need to wrap the conduit, however I opted to include the wrapper here for symmetry/safety. Please let me know if you'd prefer for this line to be reverted!
|
|
||
| @Override | ||
| public long transferFrom(FileChannel src, long position, long count) throws IOException { | ||
| if (isFinished()) { |
There was a problem hiding this comment.
Have you done any performance testing on repeated calls to isFinished()? Is there any way to memoize/cache the check? Am I optimizing prematurely? :)
There was a problem hiding this comment.
This didn't incur a measurable cost in my benchmarks. This isFinished check is identical to the isFinished check within HttpServerExchange.WriteDispatchChannel which handles a similar check at a slightly different layer.
This shouldn't have much performance impact because the underlying check is state & 2048 == 2048 where state is a standard non-volatile field.
I considered splitting the existing WriteDispatchChannel implementation so that we only need to do this check in the new conduit implementation, however that seemed riskier, and this way we do still short-circuit wrapper logic after the response has completed.
|
Trying a more precise alternative, I think this PR handles the root cause a bit better: #1648 |
|
Closing this in favor of #1648 |
Jira: https://issues.redhat.com/browse/UNDERTOW-2424
Merged PR: #1648
2.2.x PR: #1652
2.3.x PR: #1653
The failure was the result of disagreement over the owner of a connection, after an initial thread ends an exchange and releases the connection, but continues to operate upon it with a premature
flush()from an illegal thread (per undertow thread model).The HttpServerExchange uses a
WriteDispatchChannelin order to avoid interfering with the underlying connection after the exchange is completed, however theWriteDispatchChannelis an outer layer, around all response-wrappers. Response wrappers themselves may still interact with the underlying connections even after theHttpReadListenerstate has been reset to allow another request to be parsed. We must wrap the lower level streams with a similar function to prevent this spillover.In practice, the failure I've encountered and reproduced was the result of
DeflatingStreamSinkConduitinvokingnext.terminateWrites()followed bynext.flush(). TheterminateWrites()invocation causes the delegateChunkedStreamSinkConduitto flush withinterminateWriteswhich triggers the end of the exchange. Then, the followingflush()inDeflatingStreamSinkConduitapplies to another request entirely. This can be reproduced quickly using the reproducer branch (#1495) with a slight code modification, adding asleep(100)thusly:if (performFlushIfRequired()) { state |= NEXT_SHUTDOWN; freeBuffer(); next.terminateWrites(); + try { + Thread.sleep(100); + } catch (InterruptedException e) {} return next.flush(); } else {