Github user gtenev commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1100#discussion_r83510837
--- Diff: proxy/http2/Http2ConnectionState.cc ---
@@ -936,30 +940,70 @@ Http2ConnectionState::cleanup_streams()
void
Http2ConnectionState::delete_stream(Http2Stream *stream)
{
+ // The following check allows the method to be called safely on already
deleted streams.
+ if (deleted_from_active_streams(stream)) {
+ return;
+ }
+
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+
--- End diff --
If we are sure `DLL<>` is always protected by a lock then I must have
really misunderstood this previous [comment on
TS-4916](https://issues.apache.org/jira/browse/TS-4916?focusedCommentId=15552505&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15552505])
where we suspected `DLL<>` to be âmanipulated by simultaneous threadsâ.
That would mean that at least in one thread was not holding the right lock.
In that case would not that mean that ârearranging some of the stream_count
book keepingâ would rather hide the problem than to fix it?
Trying to help based on the comment decided to trace few paths in the
source code that may not be holding ConnectionState lock (theoretically) and
grabbed the lock on the common path closest to the structures that needed
protection (which based on my understanding should not be a problem if the
thread is already holding the lock).
Actually never noticed the race condition in my debugging so I am going to
remove this line from the PR and I will consider limiting my future changes to
the issue I am trying to fix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
