http-utils: cleanup the BeforeFinallyHttpOperator state #3042
Conversation
bryce-anderson
changed the title
| @@ -163,13 +172,20 @@ public void onSuccess(@Nullable final StreamingHttpResponse response) { | |||
| sendNullResponse(); | |||
| } else if (stateUpdater.compareAndSet(this, IDLE, PROCESSING_PAYLOAD)) { | |||
| subscriber.onSuccess(response.transformMessageBody(payload -> | |||
| payload.liftSync(messageBodySubscriber -> new MessageBodySubscriber(messageBodySubscriber, | |||
There was a problem hiding this comment.
We satisfy the callback requirements but we don't proactively drain the message body. Doing so will be hairy. I personally think it should be up to the receiver of the message to properly dispose of it if they no longer want it, or pass it along until something in the pipeline cares.
Opinions welcome.
There was a problem hiding this comment.
Why do you think we may need to drain message body here?
There was a problem hiding this comment.
Sorry, I certainly wasn't clear. I mean we win the race to send it but what happens if we receive a losing cancel call before the response body has been subscribed to: should we try to drain the message body at that point or is it out of our hands? I think the latter.
There was a problem hiding this comment.
Yes, it's out of our hands. For now, we will rely on operators that generate cancel, like the timeout filter, to clean it up
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/BeforeFinallyHttpOperator.java
Outdated
Show resolved
Hide resolved
bryce-anderson
changed the title
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/BeforeFinallyHttpOperator.java
Outdated
Show resolved
Hide resolved
| @@ -163,13 +172,20 @@ public void onSuccess(@Nullable final StreamingHttpResponse response) { | |||
| sendNullResponse(); | |||
| } else if (stateUpdater.compareAndSet(this, IDLE, PROCESSING_PAYLOAD)) { | |||
| subscriber.onSuccess(response.transformMessageBody(payload -> | |||
| payload.liftSync(messageBodySubscriber -> new MessageBodySubscriber(messageBodySubscriber, | |||
There was a problem hiding this comment.
Why do you think we may need to drain message body here?
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/BeforeFinallyHttpOperator.java
Show resolved
Hide resolved
…/BeforeFinallyHttpOperator.java Co-authored-by: Idel Pivnitskiy <[email protected]>
Motivation:
The BeforeFinallyHttpOperator has a few shortcomings
PROCESSING_PAYLOADstate, which has some weird callback lifetime questions.Modifications: