Add `Clients.newContextCaptor()` #2344

trustin · 2019-12-26T08:48:49Z

Motivation:

It'd be nice if a user can capture the ClientRequestContext of the
request being sent without using a decorator.

Modifications:

Add Clients.newContextCaptor()
Add ClientThreadLocalState which replaces the functional
composition of Consumer<ClientRequestContext>s.
Copy the customizer list when a DefaultClientRequestContext is
constructed, so that thread-local customizers are evaluated even if
an actual request starts from a different thread.
Add ? super to some type parameters for user convenience.
Miscellaneous:
- Fix a bug in the gRPC client where a wrong path (usually /) is set to the context.
- Make ClientRequestContext.ownAttr() nullable.

Result:

Closes Provide a way to get ClientRequestContext without using a decorator. #1739
Fixed a bug where thread-local context customizers were called for derived contexts unintentionally.
Fixed a bug where Clients.withHttpHeaders() and withContextCustomizer()
don't work for gRPC clients.
Fixed a bug in the gRPC client where a wrong path (usually /) is set to the context.

Motivation: It'd be nice if a user can capture the `ClientRequestContext` of the request being sent without using a decorator. Modifications: - Add `Clients.captureNextContext()` and `capturedContext()` - Add `ClientRequestContextCustomizers` which replaces the functional composition of `Consumer<ClientRequestContext>`s. - Copy the customizer list when a `DefaultClientRequestContext` is constructed, so that thread-local customizers are evaluated even if an actual request starts from a different thread. - Add `? super` to some type parameters for user convenience. Result: - Closes line#1739 - Fixed a bug where `Clients.withHttpHeaders()` and `withContextCustomizer()` don't work for gRPC clients.

codecov · 2019-12-26T09:04:19Z

Codecov Report

❗ No coverage uploaded for pull request base (master@69744ef). Click here to learn what that means.
The diff coverage is 55.93%.

@@            Coverage Diff            @@
##             master    #2344   +/-   ##
=========================================
  Coverage          ?   73.62%           
  Complexity        ?    10483           
=========================================
  Files             ?      928           
  Lines             ?    40101           
  Branches          ?     4944           
=========================================
  Hits              ?    29524           
  Misses            ?     8068           
  Partials          ?     2509

Impacted Files	Coverage Δ	Complexity Δ
...inecorp/armeria/client/endpoint/EndpointGroup.java	`73.52% <ø> (ø)`	`15 <0> (?)`
...corp/armeria/unsafe/grpc/GrpcUnsafeBufferUtil.java	`78.57% <ø> (ø)`	`4 <0> (?)`
...armeria/server/tomcat/Tomcat90ProtocolHandler.java	`52% <ø> (ø)`	`11 <0> (?)`
...com/linecorp/armeria/server/auth/OAuth1aToken.java	`59.01% <ø> (ø)`	`19 <0> (?)`
...a/com/linecorp/armeria/spring/ArmeriaSettings.java	`75.38% <ø> (ø)`	`13 <0> (?)`
...linecorp/armeria/server/ServiceRequestContext.java	`85% <ø> (ø)`	`9 <0> (?)`
.../connector/proxy/ArmeriaProxyConnectorFactory.java	`100% <ø> (ø)`	`7 <0> (?)`
...linecorp/armeria/server/docs/DocServiceFilter.java	`77.77% <ø> (ø)`	`24 <0> (?)`
.../com/linecorp/armeria/server/auth/OAuth2Token.java	`53.84% <ø> (ø)`	`4 <0> (?)`
...a/com/linecorp/armeria/server/auth/BasicToken.java	`61.53% <ø> (ø)`	`10 <0> (?)`
... and 33 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 69744ef...1de2333. Read the comment docs.

trustin · 2019-12-26T13:30:50Z

grpc/src/main/java/com/linecorp/armeria/client/grpc/ArmeriaChannel.java

-                uri().getRawPath(),
-                uri().getRawQuery(),
+                req.path(),
+                null,


This fixes a bug in the gRPC client where a wrong path (usually /) is set to the context. /cc @anuraaga

core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java

minwoox

👍

minwoox · 2019-12-27T05:50:58Z

core/src/test/java/com/linecorp/armeria/client/DefaultClientRequestContextTest.java

@@ -91,6 +86,36 @@ void deriveContext() {
        assertThat(derivedCtx.attr(bar).get()).isEqualTo(null);
    }

+    @Test
+    void derivedContextMustNotCallCustomizers() {


ikhoon

Awesome work! Updated two nit indentations in Javadoc 8eca4db because of your day off. 😉

core/src/main/java/com/linecorp/armeria/client/UserClient.java

ikhoon · 2019-12-27T07:20:46Z

core/src/main/java/com/linecorp/armeria/client/DefaultWebClient.java

-        return execute(null, req);
-    }
-
-    private HttpResponse execute(@Nullable EventLoop eventLoop, HttpRequest req) {


core/src/main/java/com/linecorp/armeria/client/ClientRequestContextCustomizers.java

core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java

core/src/main/java/com/linecorp/armeria/client/ClientRequestContextCustomizers.java

thrift/src/test/java/com/linecorp/armeria/client/thrift/ThriftOverHttpClientTest.java

trustin · 2019-12-30T03:09:42Z

@anuraaga @minwoox @ikhoon PTAL again.

minwoox

It looks better than before. 😉

core/src/main/java/com/linecorp/armeria/client/UserClient.java

core/src/main/java/com/linecorp/armeria/client/Clients.java

ikhoon

Still LGTM

anuraaga

LGTM code-wise but still have concern about the thread local handling for captor.

anuraaga

Thanks!

core/src/main/java/com/linecorp/armeria/client/ClientThreadLocalState.java

trustin · 2019-12-31T06:55:08Z

Added a few utility methods to ClientRequestContextCaptor - 1de2333

trustin · 2019-12-31T07:12:02Z

Thanks for reviewing!

Motivation: It'd be nice if a user can capture the `ClientRequestContext` of the request being sent without using a decorator. Modifications: - Add `Clients.newContextCaptor()` - Add `ClientThreadLocalState` which replaces the functional composition of `Consumer<ClientRequestContext>`s. - Copy the customizer list when a `DefaultClientRequestContext` is constructed, so that thread-local customizers are evaluated even if an actual request starts from a different thread. - Add `? super` to some type parameters for user convenience. - Miscellaneous: - Fix a bug in the gRPC client where a wrong path (usually /) is set to the context. - Make `ClientRequestContext.ownAttr()` nullable. Result: - Closes line#1739 - Fixed a bug where thread-local context customizers were called for derived contexts unintentionally. - Fixed a bug where `Clients.withHttpHeaders()` and `withContextCustomizer()` don't work for gRPC clients. - Fixed a bug in the gRPC client where a wrong path (usually /) is set to the context.

trustin added defect new feature labels Dec 26, 2019

trustin added this to the 0.98.0 milestone Dec 26, 2019

trustin requested review from anuraaga, ikhoon and minwoox as code owners December 26, 2019 08:48

trustin added 2 commits December 26, 2019 18:06

Fix

3ba6413

Clear after finished using

e4d2e69

trustin commented Dec 26, 2019

View reviewed changes

minwoox reviewed Dec 27, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java Show resolved Hide resolved

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

trustin added 3 commits December 27, 2019 14:30

Do not run customizers for derived contexts

a706a19

Removed unused parameter EventLoop

0ea859c

Add a test case for derived contexts

718a1b0

minwoox approved these changes Dec 27, 2019

View reviewed changes

Fix indent in Javadoc

8eca4db

ikhoon approved these changes Dec 27, 2019

View reviewed changes

anuraaga reviewed Dec 27, 2019

View reviewed changes

thrift/src/test/java/com/linecorp/armeria/client/thrift/ThriftOverHttpClientTest.java Outdated Show resolved Hide resolved

trustin added 3 commits December 30, 2019 10:53

Merge branch 'master' into capture_client_context

3641ebd

Address the comments from @anuraaga, @minwoox and @ikhoon

4022102

Renaming

708d2b2

trustin changed the title ~~Add Clients.captureNextContext() and capturedContext()~~ Add Clients.newContextCaptor() Dec 30, 2019

minwoox reviewed Dec 30, 2019

View reviewed changes

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

anuraaga reviewed Dec 30, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/Clients.java Show resolved Hide resolved

ikhoon reviewed Dec 30, 2019

View reviewed changes

trustin added 2 commits December 30, 2019 17:14

Merge branch 'master' into capture_client_context

2c9431a

Address the comments from @minwoox and @anuraaga / Cleanup

0b145d1

anuraaga approved these changes Dec 30, 2019

View reviewed changes

trustin added 2 commits December 31, 2019 15:16

Add ClientRequestContextCaptor and make it SafeCloseable

a3e4be1

Use NoSuchElementException

e09b01d

anuraaga approved these changes Dec 31, 2019

View reviewed changes

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

trustin added 3 commits December 31, 2019 15:41

Use FastThreadLocal

69744ef

Merge branch 'master' into capture_client_context

bb072f2

Add ClientRequestContextCaptor.size() and isEmpty()

1de2333

trustin merged commit 0e5cb96 into line:master Dec 31, 2019

trustin deleted the capture_client_context branch December 31, 2019 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Clients.newContextCaptor()` #2344

Add `Clients.newContextCaptor()` #2344

trustin commented Dec 26, 2019 •

edited

Loading

codecov bot commented Dec 26, 2019 •

edited

Loading

trustin Dec 26, 2019

minwoox left a comment

minwoox Dec 27, 2019

ikhoon left a comment

ikhoon Dec 27, 2019

trustin commented Dec 30, 2019

minwoox left a comment

ikhoon left a comment

anuraaga left a comment

anuraaga left a comment

trustin commented Dec 31, 2019

trustin commented Dec 31, 2019

Add Clients.newContextCaptor() #2344

Add Clients.newContextCaptor() #2344

Conversation

trustin commented Dec 26, 2019 • edited Loading

codecov bot commented Dec 26, 2019 • edited Loading

Codecov Report

trustin Dec 26, 2019

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox Dec 27, 2019

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon Dec 27, 2019

Choose a reason for hiding this comment

trustin commented Dec 30, 2019

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

trustin commented Dec 31, 2019

trustin commented Dec 31, 2019

Add `Clients.newContextCaptor()` #2344

Add `Clients.newContextCaptor()` #2344

trustin commented Dec 26, 2019 •

edited

Loading

codecov bot commented Dec 26, 2019 •

edited

Loading