Github user gtenev commented on a diff in the pull request:
    --- Diff: proxy/http2/ ---
    @@ -936,30 +940,70 @@ Http2ConnectionState::cleanup_streams()
     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 
 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 or file a JIRA ticket
with INFRA.

Reply via email to