Streams now have to be activated. This works around a race condition … #200
Conversation
…where the request kicks off before the stream container itself has had a chance to be seated by the user.
justinboswell
left a comment
justinboswell
left a comment
There was a problem hiding this comment.
Good to go other than the one issue
graebm
left a comment
graebm
left a comment
There was a problem hiding this comment.
I'm disturbed by how quick this was
| { /* BEGIN CRITICAL SECTION */ | ||
| s_h1_connection_lock_synced_data(connection); | ||
|
|
||
| if (connection->synced_data.new_stream_error_code) { | ||
| new_stream_error_code = connection->synced_data.new_stream_error_code; | ||
| } else { | ||
| aws_linked_list_push_back(&connection->synced_data.new_client_stream_list, &stream->node); | ||
| if (!connection->synced_data.is_outgoing_stream_task_active) { | ||
| connection->synced_data.is_outgoing_stream_task_active = true; | ||
| should_schedule_task = true; | ||
| } | ||
| } | ||
|
|
||
| s_h1_connection_unlock_synced_data(connection); | ||
| } /* END CRITICAL SECTION */ |
There was a problem hiding this comment.
Maybe we should move this whole critical section into activate()?
That way we only grab the lock once
I could see an argument that you'd rather see a "no new streams allowed" exception sooner than later
I'm really not sure on this one, just bringing it up.
There was a problem hiding this comment.
I'm not sure on this one either, but I suspect a fast fail if there's a connection error (such as a connection: close header received), we want this to fail sooner rather than later.
graebm
left a comment
graebm
left a comment
There was a problem hiding this comment.
Nice, we still need to add a test that creates a stream but never activates it, to be 100% sure this all cleans up right
…quest level option altogether. Added tests to verify unactivated streams still properly clean-up.
3 participants
…where the request kicks off before the stream container itself has had a chance to be seated by the user.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.