Fix memory leak in WebTransport::closed_potentially_pending_streams [chromium/src : main]
krishna dheeraj Pannala (Gerrit)
krishna dheeraj Pannala voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
krishna dheeraj Pannala (Gerrit)
Message from krishna dheeraj Pannala![]( "Open in Gerrit")
Set Ready For Review
Related details
- krishna dheeraj Pannala
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
krishna dheeraj Pannala (Gerrit)
krishna dheeraj Pannala added 1 comment![]( "Open in Gerrit")
@adam, @hayato, can you please review this CL
Related details
- Adam Rice
- Hayato Ito
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hayato Ito (Gerrit)
Hayato Ito added 1 comment![]( "Open in Gerrit")
Adam, could you please take a look at this first? I don't have any expertise in WebTransport. While this could be a good opportunity for me to familiarize myself with this area, I'm concerned it might take some time.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
known_incoming_stream_ids_.clear(); // Clear stream tracking state.nit : move comments at top of statement.
Ditto for bottom statement also
Related details
- Adam Rice
- krishna dheeraj Pannala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
HashSet<uint32_t, IntWithZeroKeyHashTraits<uint32_t>>Isn't this also effectively a memory leak? Couldn't we remove entries once we've had a close notification for them?
bool HasPendingClosedStreamForTesting(uint32_t stream_id) const {I suggest making this out-of-line, since it isn't trivial, and I don't think the check that ForTesting symbols haven't leaked into the production binary works if the function gets inlined.
Related details
- krishna dheeraj Pannala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
krishna dheeraj Pannala (Gerrit)
krishna dheeraj Pannala added 3 comments![]( "Open in Gerrit")
Isn't this also effectively a memory leak? Couldn't we remove entries once we've had a close notification for them?
I've replaced the unbounded known_incoming_stream_ids_ HashSet with a bounded LRU cache:
- recently_closed_incoming_stream_ids_ (Deque) + recently_closed_incoming_stream_ids_set_ (HashSet)
- Capped at 512 entries to cover typical QUIC stream limits (100-256 concurrent streams)
- Older entries are evicted in FIFO order
- Evicted duplicates are harmlessly treated as pending closes
This bounds memory usage while still preventing the original bug where duplicate close notifications caused unbounded growth in closed_potentially_pending_streams_.
bool HasPendingClosedStreamForTesting(uint32_t stream_id) const {I suggest making this out-of-line, since it isn't trivial, and I don't think the check that ForTesting symbols haven't leaked into the production binary works if the function gets inlined.
Done. Moved HasPendingClosedStreamForTesting() to the .cc file.
known_incoming_stream_ids_.clear(); // Clear stream tracking state.nit : move comments at top of statement.
Ditto for bottom statement also
- Adam Rice
- Hayato Ito
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
krishna dheeraj Pannala (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Adam Rice
- Hayato Ito
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 9 comments![]( "Open in Gerrit")
We now track every incoming stream ID in known_incoming_stream_ids_,CL description needs an update.
Deque<uint32_t> recently_closed_incoming_stream_ids_;It would be good to wrap these two members in a small nested class that only exposes the methods that are needed (Insert() and Contains() I think). You could call it LRUEvictionUint32Set or something similar.
"closed" is ambiguous in this context because it doesn't say who did the closing. `recently_forgotten_incoming_stream_ids_` might be a better name.
web_transport_->incoming_stream_map_.erase(stream_id);This erase() is unnecessary, and could cause the stream to be garbage collected prematurely. OnIncomingStreamClosed() will trigger a call to ForgetIncomingStream() when the stream has read all the data.
web_transport_->RecordIncomingStreamClose(stream_id);We don't need to record the `stream_id` here. We know that OnIncomingStreamClosed() has been called for this stream_id, so it won't be called again.
incoming_stream_map_.erase(it);`stream->OnIncomingStreamClosed(fin_received)` will call `erase()` via the `on_abort_` callback. There's no need to call it explicitly, although it might be good to add a comment of why we don't need to call it explicitly.
RecordIncomingStreamClose(stream_id);We don't need to record the `stream_id` here, because `OnIncomingStreamClosed()` won't be called again for the same id.
bool is_new_entry =I think `is_new_entry` must always be true, so
```
CHECK(is_new_entry);
```
RecordIncomingStreamClose(stream_id);We only need to record this if the stream we are forgetting hasn't had OnIncomingStreamClosed called on it. Probably the cleanest way to do this would be to add an extra parameter to ForgetIncomingStream() called `has_received_fin` or something like that.
// Clear stream tracking state.It is probably more efficient not to clear these sets explicitly, and just let the garbage collector free the memory.
Related details
- Hayato Ito
- krishna dheeraj Pannala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
krishna dheeraj Pannala (Gerrit)
krishna dheeraj Pannala added 11 comments![]( "Open in Gerrit")
Thanks for reviewing, Sorry for the delayed reply.
We now track every incoming stream ID in known_incoming_stream_ids_,CL description needs an update.
Done
It would be good to wrap these two members in a small nested class that only exposes the methods that are needed (Insert() and Contains() I think). You could call it LRUEvictionUint32Set or something similar.
"closed" is ambiguous in this context because it doesn't say who did the closing. `recently_forgotten_incoming_stream_ids_` might be a better name.
Done. Created `RecentlyForgottenStreamIdSet` nested class exposing only `Insert()` and `Contains()`. Named it with "FIFO eviction" (not LRU since we don't update on access). Renamed member to `recently_forgotten_incoming_stream_ids_.`
HashSet<uint32_t, IntWithZeroKeyHashTraits<uint32_t>>krishna dheeraj PannalaIsn't this also effectively a memory leak? Couldn't we remove entries once we've had a close notification for them?
I've replaced the unbounded known_incoming_stream_ids_ HashSet with a bounded LRU cache:
- recently_closed_incoming_stream_ids_ (Deque) + recently_closed_incoming_stream_ids_set_ (HashSet)
- Capped at 512 entries to cover typical QUIC stream limits (100-256 concurrent streams)
- Older entries are evicted in FIFO order
- Evicted duplicates are harmlessly treated as pending closesThis bounds memory usage while still preventing the original bug where duplicate close notifications caused unbounded growth in closed_potentially_pending_streams_.
Done. With the new logic, we only track streams forgotten before receiving `OnIncomingStreamClosed()`. Streams that receive the close notification don't get added to the tracking set, so entries are naturally limited to those that
need tracking.
This erase() is unnecessary, and could cause the stream to be garbage collected prematurely. OnIncomingStreamClosed() will trigger a call to ForgetIncomingStream() when the stream has read all the data.
Done. Removed the premature `erase()`. Added comment explaining that cleanup happens via the `on_abort_` callback to `ForgetIncomingStream()` .
We don't need to record the `stream_id` here. We know that OnIncomingStreamClosed() has been called for this stream_id, so it won't be called again.
Done. Removed `RecordIncomingStreamClose()` call here.
`stream->OnIncomingStreamClosed(fin_received)` will call `erase()` via the `on_abort_` callback. There's no need to call it explicitly, although it might be good to add a comment of why we don't need to call it explicitly.
Done. Removed the explicit `erase()` and added comment:
> "Note: stream-> OnIncomingStreamClosed() will eventually trigger ForgetIncomingStream() via the on_abort_ callback, which handles removal from incoming_stream_map_."
We don't need to record the `stream_id` here, because `OnIncomingStreamClosed()` won't be called again for the same id.
Done. Removed `RecordIncomingStreamClose()` call. Added comment explaining we don't need to record because `OnIncomingStreamClosed()` won't be called again for the same `stream_id`.
I think `is_new_entry` must always be true, so
```
CHECK(is_new_entry);
```
Done. Added CHECK(result.is_new_entry); with comment
> "Should always be new given our call sites."
We only need to record this if the stream we are forgetting hasn't had OnIncomingStreamClosed called on it. Probably the cleanest way to do this would be to add an extra parameter to ForgetIncomingStream() called `has_received_fin` or something like that.
Done. Added `bool has_received_close` parameter to `ForgetIncomingStream()`. Updated on_abort_ callback signature to pass `fin_received_.has_value()` from `AbortAndReset()`. Only insert into tracking set when `!has_received_close`.
It is probably more efficient not to clear these sets explicitly, and just let the garbage collector free the memory.
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 5 comments![]( "Open in Gerrit")
Almost done. One functional change and some nits.
Deque<uint32_t> ids_;I think you can make the implementation of this simpler by using `LinkedHashSet<uint32_t>` as the underlying data structure. Then eviction can be implemented as
```
if (id_set_.size() > kMaxSize) {
id_set_.RemoveFirst();
}
```
This will also allow you to easily implement `Erase(id)` so that once an element has been found in `OnIncomingStreamClosed()` you can immediately remove it from the set, since you know `OnIncomingStreamClosed()` will not be called again for the same ID.
Nit: it's good practice to protect against accidental copies. Even when the class is private, someone might add code that accidentally copies it in future.
```
RecentlyForgottenStreamIdSet() = default;
RecentlyForgottenStreamIdSet(const RecentlyForgottenStreamIdSet&) = delete;
RecentlyForgottenStreamIdSet& operator=(const RecentlyForgottenStreamIdSet&) = delete;
```
// closed_potentially_pending_streams_ are properly consumed or cleared.Nit: put `closed_potentially_pending_streams_` in backticks so that it will be linkified in code search.
```suggestion
// `closed_potentially_pending_streams_` are properly consumed or cleared.
```
DVLOG(1) << "WebTransport::OnIncomingStreamClosed() ignoring duplicate "Nit: It's not really a "duplicate". It might be better to have a message like
"WebTransport::OnIncomingStreamClosed() correctly ignoring close on recently forgotten stream stream_id=". The word "correctly" is just so that somebody who sees the message doesn't mistake it for an error.
Related details
- krishna dheeraj Pannala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
krishna dheeraj Pannala (Gerrit)
krishna dheeraj Pannala added 4 comments![]( "Open in Gerrit")
I think you can make the implementation of this simpler by using `LinkedHashSet<uint32_t>` as the underlying data structure. Then eviction can be implemented as
```
if (id_set_.size() > kMaxSize) {
id_set_.RemoveFirst();
}
```
This will also allow you to easily implement `Erase(id)` so that once an element has been found in `OnIncomingStreamClosed()` you can immediately remove it from the set, since you know `OnIncomingStreamClosed()` will not be called again for the same ID.
Refactored to use LinkedHashSet for FIFO eviction.
Nit: it's good practice to protect against accidental copies. Even when the class is private, someone might add code that accidentally copies it in future.
```
RecentlyForgottenStreamIdSet() = default;
RecentlyForgottenStreamIdSet(const RecentlyForgottenStreamIdSet&) = delete;
RecentlyForgottenStreamIdSet& operator=(const RecentlyForgottenStreamIdSet&) = delete;
```
Good point. Added explicit deletion of copy constructor and copy assignment operator to guard against accidental copies in future code changes.
// closed_potentially_pending_streams_ are properly consumed or cleared.Nit: put `closed_potentially_pending_streams_` in backticks so that it will be linkified in code search.
```suggestion
// `closed_potentially_pending_streams_` are properly consumed or cleared.
```
Done
DVLOG(1) << "WebTransport::OnIncomingStreamClosed() ignoring duplicate "Nit: It's not really a "duplicate". It might be better to have a message like
"WebTransport::OnIncomingStreamClosed() correctly ignoring close on recently forgotten stream stream_id=". The word "correctly" is just so that somebody who sees the message doesn't mistake it for an error.
Updated the message to:
"WebTransport::OnIncomingStreamClosed() correctly ignoring close on recently forgotten stream stream_id=..."
The word "correctly" makes it clear this is expected behavior, not an error.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |