Closed jrhee17 closed 3 months ago
https://github.com/line/armeria/pull/5156 has been merged. Would you mind resolving the conflicts?
@jrhee17 Would you check the test failures?
ContextCancellationTest > cancel_beforeDelegate(TestInfo) FAILED
java.lang.AssertionError:
Expecting empty but was: [[creqId=147071ed][http://127.0.0.1:34223/#GET]]
at com.linecorp.armeria.client.ContextCancellationTest.validateCallbackChecks(ContextCancellationTest.java:289)
at com.linecorp.armeria.client.ContextCancellationTest.cancel_beforeDelegate(ContextCancellationTest.java:115)
ContextCancellationTest > cancel_beforeConnection(TestInfo) FAILED
java.lang.AssertionError:
Expecting empty but was: [[creqId=456bf5ae][http://foo.com:34223/#POST]]
at com.linecorp.armeria.client.ContextCancellationTest.validateCallbackChecks(ContextCancellationTest.java:289)
at com.linecorp.armeria.client.ContextCancellationTest.cancel_beforeConnection(ContextCancellationTest.java:153)
Let me go ahead and merge this PR since it's reliable passing the CI again, thanks for the review all 👍
Motivation:
In order to handle https://github.com/line/armeria/issues/4591, I propose that we first define an API which allows users to cancel a request. Currently,
ClientRequestContext#cancel
is invoked onceCancellationScheduler#start
is invoked. I propose to change the behavior ofClientRequestContext#cancel
such that the associated request is aborted immediately. Once this API is available, it would be trivial to implementResponseTimeoutMode
by adjusting where to callCancellationScheduler#start
. Additionally, users may easily implement their own timeout mechanism if they would like.Modifications:
CancellationScheduler
related changesDefaultCancellationScheduler
is refactored to be lock based instead of event loop based. The reasoning for this change was for the scenario where the request execution didn't reach the event loop yet. i.e. If a user callsctx.cancel
in the decorator, a connection shouldn't be opened.CancellationScheduler#updateTask
is introduced. This API updates the cancellation task if the scheduler isn't invoked yet. If the scheduler is invoked already, the cancellation task will be executed eventually. This API allowsctx.cancel
to attempt cancellation depending on which stage the request is at. For instance, at the decorators onlyreq.abort
needs to be called but at the request write stage, the cancellation task may need to send a reset signal.Long.MAX_VALUE
instead of0
AbstractHttpRequestHandler
related changesCancellationTask
inAbstractHttpRequestHandler
,HttpResponseWrapper
,AbstractHttpResponseHandler
is modified to be scheduled inside an event loop. The reasoning is thatctx.cancel
, and henceCancellationTask#run
can be invoked from any thread.AbstractHttpRequestHandler
callingfail
orfailAndReset
multiple times. There is no point in doing so, so added a boolean flagfailed
for safety.HttpResponseWrapper
related changescancelTimeoutAndLog
was to not log if the response is unexpected. Modified so that if the response is cancelled or the context is cancelled, no logging is doneclose
when a timeout occurs. Unified the fragmented logic of closing theHttpResponseWrapper
.DefaultRequestLog
is completing futures with the context set on the event loop. We generally don't do this since we strictly validate against setting incorrectRequestContext
s, so modified to usewithoutContext
.Result:
ClientRequestContext#cancel
to cancel the ongoing request easily.