HttpResponse.of(ResponseHeaders,Publisher<HttpObject>) #3237

tumile · 2020-12-18T11:27:31Z

Motivation:
Provide an easy way to build a HttpResponse with ResponseHeaders and a Publisher<HttpObject>.

Modifications:
Added PrependingPublisher that can prepend an object to a Publisher of the same type.

Result:
Closes #3089

tumile · 2020-12-18T12:00:29Z

Not sure why this happens, it seems irrelevant 😄
https://ci.appveyor.com/project/line/armeria/builds/36905264/job/6pv80j0idqg0g8vd#L638

11:41:07.703 [Thread-3] DEBUG c.l.armeria.client.ClientFactory - Closed the default client factories
TomcatServiceDestroyedConnectorTest > serviceUnavailableAfterConnectorIsDestroyed() FAILED
    java.lang.AssertionError: 
    Expecting:
     <200 OK>
    and actual:
     <503 Service Unavailable>
    to refer to the same object
        at com.linecorp.armeria.server.tomcat.TomcatServiceDestroyedConnectorTest.serviceUnavailableAfterConnectorIsDestroyed(TomcatServiceDestroyedConnectorTest.java:55)

minwoox · 2020-12-18T12:37:17Z

Please forget about it. It's a flaky test. 😅

trustin

Nice! Sorry for a late review, @tumile. Have a great holiday ☃️

core/src/main/java/com/linecorp/armeria/common/HttpResponse.java

core/src/main/java/com/linecorp/armeria/common/PublisherBasedHttpResponse.java

codecov · 2020-12-24T08:14:19Z

Codecov Report

Merging #3237 (39302b8) into master (a62e8d8) will increase coverage by 0.01%.
The diff coverage is 56.96%.

@@             Coverage Diff              @@
##             master    #3237      +/-   ##
============================================
+ Coverage     74.11%   74.13%   +0.01%     
- Complexity    12873    13244     +371     
============================================
  Files          1119     1158      +39     
  Lines         48746    50367    +1621     
  Branches       6229     6442     +213     
============================================
+ Hits          36129    37339    +1210     
- Misses         9437     9741     +304     
- Partials       3180     3287     +107

Impacted Files	Coverage Δ	Complexity Δ
...rp/armeria/client/ClientRequestContextWrapper.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...linecorp/armeria/common/RequestContextWrapper.java	`7.50% <0.00%> (+7.50%)`	`1.00 <0.00> (+1.00)`
...p/armeria/server/ServiceRequestContextWrapper.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../linecorp/armeria/server/tomcat/TomcatService.java	`58.74% <28.57%> (-2.11%)`	`32.00 <2.00> (+2.00)`	⬇️
...va/com/linecorp/armeria/internal/consul/Check.java	`33.33% <33.33%> (ø)`	`27.00 <27.00> (?)`
...inecorp/armeria/internal/consul/CatalogClient.java	`50.00% <54.16%> (ø)`	`9.00 <9.00> (?)`
...ia/internal/common/stream/PrependingPublisher.java	`55.71% <55.71%> (ø)`	`2.00 <2.00> (?)`
.../armeria/server/consul/ConsulUpdatingListener.java	`58.20% <56.92%> (ø)`	`12.00 <12.00> (?)`
...linecorp/armeria/internal/consul/HealthClient.java	`60.97% <60.97%> (ø)`	`6.00 <6.00> (?)`
...eria/client/consul/ConsulEndpointGroupBuilder.java	`76.00% <66.66%> (ø)`	`8.00 <4.00> (?)`
... and 177 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 ed83cab...39302b8. Read the comment docs.

tumile · 2020-12-26T10:30:32Z

@trustin Hope you and the team have a great holiday too! And hope that Armeria has another year of wonderful growth!

But to start it out we have another npmInstall failure 🤣. Just out of curiosity why are we not keeping site as a separate repository like https://github.com/spring-io/sagan?

trustin · 2020-12-28T02:21:43Z

Just out of curiosity why are we not keeping site as a separate repository like spring-io/sagan?

Sorry about the inconveniences. 😢 It's because we want to refer to some type information in our documentation.

trustin

👍

core/src/main/java/com/linecorp/armeria/common/PublisherBasedHttpResponse.java

ikhoon

Left some comments that violate Reactive Streams Specifications. Those things could be checked by Reactive Streams TCK.

core/src/main/java/com/linecorp/armeria/internal/common/stream/PrependingPublisher.java

core/src/test/java/com/linecorp/armeria/internal/common/stream/PrependingPublisherTckTest.java

core/src/main/java/com/linecorp/armeria/internal/common/stream/PrependingPublisher.java

core/src/main/java/com/linecorp/armeria/common/HttpResponse.java

core/src/main/java/com/linecorp/armeria/common/PublisherBasedHttpResponse.java

core/src/main/java/com/linecorp/armeria/internal/common/stream/PrependingPublisher.java

trustin

Still LGTM, once @minwoox's comments are addressed.

ikhoon

Nice work! @tumile

core/src/main/java/com/linecorp/armeria/internal/common/stream/PrependingPublisher.java

minwoox

I think we are almost there. 😄

core/src/main/java/com/linecorp/armeria/internal/common/stream/PrependingPublisher.java

minwoox · 2021-01-25T03:56:50Z

core/src/main/java/com/linecorp/armeria/internal/common/stream/PrependingPublisher.java

+                final long demand = this.demand;
+                final long newDemand = demand >= Long.MAX_VALUE - n ? Long.MAX_VALUE : demand + n;
+                if (demandUpdater.compareAndSet(this, demand, newDemand)) {
+                    if (demand > 0) {


Shouldn't we just remove this condition?

We check this because if the previous demand is non-zero then there is already an ongoing request and we stop, otherwise it could lead to unbounded recursion request -> onNext -> request ->.... If I remove this rule 3.3 will fail 😄 .

Oops could you elaborate more on request -> onNext -> request ->..., please?
If we just return here, there's a chance the upstream.request(...) is not called.
Let's say that thread A calls Subscription.request(2) twice.
When the second Subscription.request(2) is called and if it's at line 84.
Meanwhile, thread B is trying demandUpdater.compareAndSet(this, demand, 0) at line 138.
If the thread B fails to compareAndSet, then thread A might set the demand as 3 and there's no more upstream.request(demand); call

Could the race condition be solved by adding a for loop in onSubscribe()?

// Keep retrying to send a request signal until demain is zero for (;;) { final long demand = this.demand; if (demand == 0) { break; } if (demandUpdater.compareAndSet(this, demand, 0)) { subscription.request(demand); } }

@minwoox thanks for the detailed example. There's indeed a race condition. I think @ikhoon's solution should resolve that. Yet if (demand > 0) is for guarding stack overflow, so it should still be here? 😄

I probably might get this wrong, 😄 but if's it's for guarding a stack overflow,
what happens when the thread calls upstream.request(demand) here?https://github.com/line/armeria/pull/3237/files#diff-a8dfc683de291a0ccbc6e0ab3c2bae463e462050ac18beca18b3adf6c14c1b5fR110
Isn't it the same?

Right, I get what you say 😄 . The condition is there to satisfy the TCK, which assumes the calls are on the same thread. In fact rule 3.3 just "recommended" the stack depth to be 1. I don't think in practice we would overflow by any means, especially in Armeria.

core/src/main/java/com/linecorp/armeria/internal/common/stream/PrependingPublisher.java

minwoox

Anyway, thanks a lot for your patience, @tumile. 😄

tumile · 2021-02-01T08:21:18Z

Thanks everyone! I didn't think this could turn out to be so complicated. Couldn't have finished it without your support 🙇

minwoox · 2021-02-01T08:45:45Z

I didn't think this could turn out to be so complicated.

Neither did I. 🤣

Build response from headers and HttpData publisher

aa07137

tumile requested review from ikhoon, minwoox and trustin as code owners December 18, 2020 11:27

trustin added the new feature label Dec 24, 2020

trustin added this to the 1.4.0 milestone Dec 24, 2020

trustin reviewed Dec 24, 2020

View reviewed changes

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

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

Address reviews

73ae268

trustin approved these changes Dec 28, 2020

View reviewed changes

minwoox reviewed Dec 28, 2020

View reviewed changes

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

Fix onComplete called before subscribed

c5ce195

ikhoon reviewed Dec 31, 2020

View reviewed changes

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

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

Add PrependingPublisher

3838c27

ikhoon reviewed Jan 4, 2021

View reviewed changes

Add PrependingPublisherTckTest

afd7938

ikhoon reviewed Jan 6, 2021

View reviewed changes

Address reviews by @ikhoon

bddfd66

ikhoon reviewed Jan 15, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/common/stream/PrependingPublisher.java Show resolved Hide resolved

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

Address reviews

15f5711

minwoox reviewed Jan 20, 2021

View reviewed changes

trustin approved these changes Jan 20, 2021

View reviewed changes

Address reviews by @minwoox

c64f698

ikhoon approved these changes Jan 25, 2021

View reviewed changes

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

minwoox reviewed Jan 25, 2021

View reviewed changes

Finishing up

f458106

Fix

39302b8

minwoox approved these changes Feb 1, 2021

View reviewed changes

trustin changed the title ~~HttpResponse.of(ResponseHeaders,Publisher<HttpData>)~~ HttpResponse.of(ResponseHeaders,Publisher<HttpObject>) Feb 1, 2021

trustin merged commit 4d0aa33 into line:master Feb 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpResponse.of(ResponseHeaders,Publisher<HttpObject>) #3237

HttpResponse.of(ResponseHeaders,Publisher<HttpObject>) #3237

tumile commented Dec 18, 2020 •

edited

Loading

tumile commented Dec 18, 2020

minwoox commented Dec 18, 2020

trustin left a comment

codecov bot commented Dec 24, 2020 •

edited

Loading

tumile commented Dec 26, 2020

trustin commented Dec 28, 2020

trustin left a comment

ikhoon left a comment •

edited

Loading

trustin left a comment

ikhoon left a comment

minwoox left a comment

minwoox Jan 25, 2021

tumile Jan 25, 2021 •

edited

Loading

minwoox Jan 28, 2021 •

edited

Loading

ikhoon Jan 28, 2021

tumile Jan 29, 2021 •

edited

Loading

minwoox Jan 31, 2021

tumile Feb 1, 2021 •

edited

Loading

minwoox left a comment

tumile commented Feb 1, 2021

minwoox commented Feb 1, 2021

HttpResponse.of(ResponseHeaders,Publisher<HttpObject>) #3237

HttpResponse.of(ResponseHeaders,Publisher<HttpObject>) #3237

Conversation

tumile commented Dec 18, 2020 • edited Loading

tumile commented Dec 18, 2020

minwoox commented Dec 18, 2020

trustin left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 24, 2020 • edited Loading

Codecov Report

tumile commented Dec 26, 2020

trustin commented Dec 28, 2020

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment • edited Loading

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox Jan 25, 2021

Choose a reason for hiding this comment

tumile Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

minwoox Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

ikhoon Jan 28, 2021

Choose a reason for hiding this comment

tumile Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

minwoox Jan 31, 2021

Choose a reason for hiding this comment

tumile Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

tumile commented Feb 1, 2021

minwoox commented Feb 1, 2021

tumile commented Dec 18, 2020 •

edited

Loading

codecov bot commented Dec 24, 2020 •

edited

Loading

ikhoon left a comment •

edited

Loading

tumile Jan 25, 2021 •

edited

Loading

minwoox Jan 28, 2021 •

edited

Loading

tumile Jan 29, 2021 •

edited

Loading

tumile Feb 1, 2021 •

edited

Loading