Add FilteredStreamMessage.onCancellation() #3375

minwoox · 2021-03-09T02:56:20Z

Motivation:
An implementation of the FilteredStreamMessage might have resources to release when
the StreamMessage is complete. For example, an HttpResponse from ContentPreviewingService should call
ContentPreviewer.produce() to release the resource and produce the content preview.
However, because we don't provide a hook for Subscription.cancel(), it's easy to forget to clean up resources when the stream message is canceled.

Modification:

Add FilteredStreamMessage.onCancellation().

Result:

You can now clean up resources by overriding FilteredStreamMessage.onCancellation(...) when Subscription.cancel() is called.

new MyFileteredHttpResponse(res) {

    ...
    @Override
    protected void onCancellation(Subscriber<? super U> subscriber) {
        // Clean up resources.
    }
}

You no longer see that the log is not complete when applying ContentPreviewingService.

Motivation: An implementation of the `FilteredStreamMessage` might have resources to release when the `StreamMessage` is complete. For example, an `HttpResponse` from `ContentPreviewingService` should call `ContentPreviewer.produce()` to release the resource and produce the content preview. However, the `FilteredStreamMessage` is not notified when the `StreamMessage` is canceled by the `Subscriber`. Modification: - Add `FilteredStreamMessage.beforeCancel()` so that an implementation of the message can add a hook before `Subscription.cancel()`. Result: - You no longer see that the log is not complete when applying `ContentPreviewingService`.

codecov · 2021-03-09T03:20:53Z

Codecov Report

Merging #3375 (986c488) into master (9fa89e3) will decrease coverage by 0.01%.
The diff coverage is 69.04%.

❗ Current head 986c488 differs from pull request most recent head 3cdc0b3. Consider uploading reports for the commit 3cdc0b3 to get more accurate results

@@             Coverage Diff              @@
##             master    #3375      +/-   ##
============================================
- Coverage     74.31%   74.29%   -0.02%     
- Complexity    13527    13534       +7     
============================================
  Files          1174     1174              
  Lines         51703    51744      +41     
  Branches       6631     6641      +10     
============================================
+ Hits          38422    38443      +21     
- Misses         9915     9931      +16     
- Partials       3366     3370       +4

Impacted Files	Coverage Δ	Complexity Δ
...internal/client/grpc/GrpcWebTrailersExtractor.java	`70.00% <ø> (-1.63%)`	`3.00 <0.00> (ø)`
...p/armeria/server/encoding/HttpEncodedResponse.java	`65.33% <16.66%> (-3.24%)`	`17.00 <2.00> (ø)`
...rp/armeria/server/encoding/HttpDecodedRequest.java	`44.00% <40.00%> (-6.00%)`	`5.00 <3.00> (+2.00)`	⬇️
...p/armeria/client/encoding/HttpDecodedResponse.java	`67.34% <75.00%> (+5.18%)`	`14.00 <7.00> (+4.00)`
...rmeria/internal/logging/ContentPreviewingUtil.java	`85.45% <80.00%> (+11.03%)`	`6.00 <2.00> (+2.00)`
...p/armeria/common/stream/FilteredStreamMessage.java	`81.81% <91.66%> (-1.97%)`	`16.00 <3.00> (-1.00)`
...meria/common/stream/RegularFixedStreamMessage.java	`88.67% <0.00%> (-5.67%)`	`14.00% <0.00%> (-1.00%)`
...orp/armeria/client/eureka/EurekaEndpointGroup.java	`60.71% <0.00%> (-3.58%)`	`23.00% <0.00%> (-1.00%)`
...ia/common/stream/ConcatPublisherStreamMessage.java	`77.09% <0.00%> (-3.06%)`	`8.00% <0.00%> (ø%)`
...eria/internal/common/DefaultSplitHttpResponse.java	`77.38% <0.00%> (-1.79%)`	`14.00% <0.00%> (ø%)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fa89e3...3cdc0b3. Read the comment docs.

ikhoon

Could you check this FilteredStreamMessage?

armeria/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcWebTrailersExtractor.java

Lines 133 to 142 in bd09b15

    
           @Override 
        
           protected void beforeComplete(Subscriber<? super HttpObject> subscriber) { 
        
               publisher.close(); 
        
           } 
        
           @Override 
        
           protected Throwable beforeError(Subscriber<? super HttpObject> subscriber, Throwable cause) { 
        
               publisher.close(); 
        
               return cause; 
        
           }

core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java

minwoox · 2021-03-09T06:21:47Z

Could you check this FilteredStreamMessage?

This one? 😄 https://github.com/line/armeria/pull/3375/files#diff-0232a75a1b4ec25d2ba5bd59312da7df3db4dac369cc2949331e6dc837c59418R146

ikhoon · 2021-03-09T08:35:07Z

Could you check this FilteredStreamMessage?

This one? 😄 #3375 (files)

Indeed it is. 😅

ikhoon · 2021-03-09T08:46:42Z

core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java

-                    // Call requestContentPreview(null) to make sure that the log is complete.
-                    ctx.logBuilder().responseContentPreview(null);
-                }
+                produceResponseContentPreview(responseContentPreviewer);


Is there any chance that subscriber.onComplete() and subscription.cancel() are called concurrently?

Maybe we could have an atomic flag, then we won't even need the cause instanceof CancelledSubscriptionException checks.

I realized that if we introduce an atomic flag, a user needs to do the extra work because beforeCancel() and beforeError() can be called twice in certain situation.
So let me just remove beforeCancel() and handle the cancelation event in beforeError.

core/src/main/java/com/linecorp/armeria/server/encoding/HttpDecodedRequest.java

core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncodedResponse.java

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcWebTrailersExtractor.java

core/src/main/java/com/linecorp/armeria/common/stream/FilteredStreamMessage.java

core/src/main/java/com/linecorp/armeria/client/encoding/HttpDecodedResponse.java

ikhoon

Thanks a lot! @minwoox

core/src/test/java/com/linecorp/armeria/server/DefaultServiceRequestContextTest.java

ikhoon · 2021-03-26T02:58:41Z

Would you mind resolving the conflicting files? 😄

minwoox · 2021-03-26T03:10:09Z

Thanks fixed. 😉

core/src/main/java/com/linecorp/armeria/client/encoding/HttpDecodedResponse.java

core/src/main/java/com/linecorp/armeria/server/encoding/HttpDecodedRequest.java

trustin

Thanks, @minwoox !

minwoox · 2021-03-26T09:18:18Z

Thanks for reviewing. 😉

minwoox added defect new feature labels Mar 9, 2021

minwoox added this to the 1.6.0 milestone Mar 9, 2021

minwoox requested review from ikhoon and trustin as code owners March 9, 2021 02:56

ikhoon reviewed Mar 9, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java Outdated Show resolved Hide resolved

minwoox added 2 commits March 9, 2021 15:30

Add Subscriber to beforeCancel

dceb402

Address the comment by @ikhoon

56a9423

ikhoon reviewed Mar 9, 2021

View reviewed changes

trustin reviewed Mar 9, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/encoding/HttpDecodedRequest.java Outdated Show resolved Hide resolved

trustin reviewed Mar 9, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncodedResponse.java Show resolved Hide resolved

trustin reviewed Mar 9, 2021

View reviewed changes

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcWebTrailersExtractor.java Outdated Show resolved Hide resolved

minwoox added 2 commits March 9, 2021 22:49

Remove beforeCancel and handle in beforeError

052bc0a

Fix comments

0b27e91

minwoox removed the new feature label Mar 9, 2021

minwoox changed the title ~~Add beforeCancel hook to FilteredStreamMessage~~ Call FilteredStreamMessage.beforeError() when the StreamMessage is canceled Mar 9, 2021

Update Javadoc

3b2e245

ikhoon reviewed Mar 10, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/FilteredStreamMessage.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/stream/FilteredStreamMessage.java Outdated Show resolved Hide resolved

minwoox added 2 commits March 11, 2021 10:00

Rephrease Javadoc

d4ab16d

Use whenComplete to clean up resources

232b241

ikhoon reviewed Mar 11, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/encoding/HttpDecodedResponse.java Outdated Show resolved Hide resolved

Add onCancellation

1379526

minwoox added the new feature label Mar 11, 2021

ikhoon approved these changes Mar 11, 2021

View reviewed changes

minwoox changed the title ~~Call FilteredStreamMessage.beforeError() when the StreamMessage is canceled~~ Add FilteredStreamMessage.onCancellation() Mar 11, 2021

Remove Subscriber

a0b99a9

minwoox added 2 commits March 25, 2021 11:51

Address the comment by @trustin

cd89b76

Fix flaky

5245539

ikhoon reviewed Mar 26, 2021

View reviewed changes

core/src/test/java/com/linecorp/armeria/server/DefaultServiceRequestContextTest.java Show resolved Hide resolved

Merge branch 'master' into addBeforeCancel

2a70d7f

Fix compile

fe7cd85

trustin reviewed Mar 26, 2021

View reviewed changes

Address comments by @trustin

3cdc0b3

trustin approved these changes Mar 26, 2021

View reviewed changes

trustin merged commit 386268a into line:master Mar 26, 2021

minwoox deleted the addBeforeCancel branch March 26, 2021 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FilteredStreamMessage.onCancellation() #3375

Add FilteredStreamMessage.onCancellation() #3375

minwoox commented Mar 9, 2021 •

edited

codecov bot commented Mar 9, 2021 •

edited

ikhoon left a comment

minwoox commented Mar 9, 2021

ikhoon commented Mar 9, 2021

ikhoon Mar 9, 2021

trustin Mar 9, 2021

minwoox Mar 9, 2021

ikhoon left a comment

ikhoon commented Mar 26, 2021

minwoox commented Mar 26, 2021

trustin left a comment

minwoox commented Mar 26, 2021

	@Override
	protected void beforeComplete(Subscriber<? super HttpObject> subscriber) {
	publisher.close();
	}

	@Override
	protected Throwable beforeError(Subscriber<? super HttpObject> subscriber, Throwable cause) {
	publisher.close();
	return cause;
	}

Add FilteredStreamMessage.onCancellation() #3375

Add FilteredStreamMessage.onCancellation() #3375

Conversation

minwoox commented Mar 9, 2021 • edited

codecov bot commented Mar 9, 2021 • edited

Codecov Report

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Mar 9, 2021

ikhoon commented Mar 9, 2021

ikhoon Mar 9, 2021

Choose a reason for hiding this comment

trustin Mar 9, 2021

Choose a reason for hiding this comment

minwoox Mar 9, 2021

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon commented Mar 26, 2021

minwoox commented Mar 26, 2021

trustin left a comment

Choose a reason for hiding this comment

minwoox commented Mar 26, 2021

minwoox commented Mar 9, 2021 •

edited

codecov bot commented Mar 9, 2021 •

edited