Fix refcounting bug in h2_connection.c #236
Conversation
- aws_stream_activate() should be incrementing stream refcount, not the function that moves it to a thread.
- renamed the function that moves it to a thread to reduce confusion.
- reduce number of times we load atomic new_stream_error_code.
- moved atomics out of `synced_data` to reduce confusion
| struct aws_h2_stream *stream = AWS_CONTAINER_OF(node, struct aws_h2_stream, node); | ||
| s_activate_stream(connection, stream); | ||
| if (!aws_linked_list_empty(&pending_streams)) { | ||
| int new_stream_error_code = (int)aws_atomic_load_int(&connection->atomic.new_stream_error_code); |
There was a problem hiding this comment.
Question: Why we fetch the error code here? So when the connection closes while we are iterating the pending_streams, it will not be updated, right? To improve performance?
There was a problem hiding this comment.
Yes, to improve performance. It's 1 atomic_load for the N streams being activated, vs N atomic_loads.
It's ok if we're not perfectly in sync here. Eventually, the connection shuts down and any streams remaining in synced_data.pending_streams or thread_data.active_streams_map are completed with AWS_ERROR_HTTP_CONNECTION_CLOSED. This function is running on the event-loop thread, and so are (almost) all the other things that might set the atomic.new_stream_error_code.
The only other thing that can set it is a user calling aws_http_connection_close(). So, if a user calls close() in the middle of this loop running, then the streams will still activate successfully, but most likely they'll fail when the connection shuts down over the next few ticks of the event-loop.
- aws_stream_activate() should be incrementing stream refcount, not the function that moves it to a thread.
- renamed the function that moves it to a thread to reduce confusion.
- reduce number of times we load atomic new_stream_error_code.
- moved atomics out of `synced_data` to reduce confusion
2 participants
s_stream_activate()->s_move_stream_to_thread()so it's not confused withaws_h2_stream_activate()new_stream_error_code.synced_datato reduce confusionBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.