AbortStream method to Http1StreamManager and ensure streams are released to connection pool#1002
AbortStream method to Http1StreamManager and ensure streams are released to connection pool#1002TingDaoK wants to merge 12 commits into
Conversation
…eleased to pool in all cases.
| streamHandler.onResponseComplete(stream, e.errorCode); | ||
| /* Release the connection back */ | ||
| connManager.releaseConnection(conn); | ||
| if (connectionReleased.compareAndSet(false, true)) { |
There was a problem hiding this comment.
should we allow multiple release of connections natively?
There was a problem hiding this comment.
Looks like we do allow it. Why should connection be released exactly once?
There was a problem hiding this comment.
it actually ends up with httpClientConnectionReleaseManaged in the binding, it avoids the race between multiple threads trying to release the same connection back to the pool.
aws_http_connection_manager_release_connection cannot be invoked twice.
And the binding level is not thread safe.
| * | ||
| * <p><b>Important:</b> Operations on the stream (such as {@code cancel()}) must only be invoked | ||
| * after {@code activate()} has been called. If {@code cancel()} is called before activate, it | ||
| * is a no-op — the stream will still proceed normally once activate runs, which may lead to |
There was a problem hiding this comment.
a bit more of a philosophical question, but should activate on cancelled stream really be a noop? should it not activate the stream at all?
There was a problem hiding this comment.
well, yeah, the activate on the cancelled stream should result in no-op.
But, we kind designed the cancel on an unactivated stream to be a no-op. And without altering any state of the stream. https://github.com/awslabs/aws-c-http/blob/main/include/aws/http/request_response.h#L1222-L1232
If we keep that behavior to basically ignore the cancel, then it still makes more sense to let activate do its thing.
There was a problem hiding this comment.
in retrospect was it the correct choice to design it this way? not saying we should change it now.
cancel being a noop for non-active connection seems to bring more problems that we have to work around.
TingDaoK
changed the title
4 participants
Issue #, if available:
Description of changes:
Otherside of the original PR, which already be well described in the original description.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.