TS-4916 Add safety net to avoid H2-infinite-loop deadlock.#1117
TS-4916 Add safety net to avoid H2-infinite-loop deadlock.#1117zwoop merged 1 commit intoapache:masterfrom gtenev:TS-4916-master
Conversation
|
Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/932/ for details. |
|
👍 - Looks good |
proxy/http2/Http2Stream.cc
Outdated
| // less error prone to (double) delete before destroying (noop if already deleted). | ||
| if (parent) { | ||
| static_cast<Http2ClientSession *>(parent)->connection_state.delete_stream(this); | ||
| Warning("Http2Stream was about to be deallocated without removing it from the active stream list"); |
There was a problem hiding this comment.
We are sure this is not going to produce a lot of warnings, right ? :)
There was a problem hiding this comment.
If the streams are removed properly from the active stream list before this catch-all-delete-stream call and freeing the stream's memory we should not see this at all.
With the current 6.2.1 code it took 1-3 days for this to occur so even if the problem persists (not deleting the streams on time) it seems reasonable to believe we would see the warning with similar frequency (once in 1-3 days).
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1041/ for details. |
|
Sigh, it failed again on the bison generated files :-/. [approve ci]. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1048/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/940/ for details. |
Current Http2ConnectionState implementation uses a memory pool for instantiating streams and DLL<> stream_list for storing active streams. Destroying a stream before deleting it from stream_list and then creating a new one + reusing the same chunk from the memory pool right away always leads to destroying the DLL structure (deadlocks, inconsistencies). Added a safety net since the consequences are disastrous. Until the design/implementation changes it seems less error prone to (double) delete before destroying (noop if already deleted).
|
Looks good to me as well. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1050/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/942/ for details. |
Current Http2ConnectionState implementation uses a memory pool for
instantiating streams and DLL<> stream_list for storing active streams.
Destroying a stream before deleting it from stream_list and then creating
a new one + reusing the same chunk from the memory pool right away always
leads to destroying the DLL structure (deadlocks, inconsistencies).
Added a safety net since the consequences are disastrous.
Until the design/implementation changes it seems less error prone
to (double) delete before destroying (noop if already deleted).