Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abide by the rule Reactive Streams 2.3 #2815

Merged
merged 2 commits into from Jun 19, 2020

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Jun 18, 2020

Some of the subscribers violated Reactive Stremas 2.3 which is:
Subscriber.onComplete() and Subscriber.onError(Throwable t) MUST NOT call any methods on the Subscription or the Publisher.

https://github.com/reactive-streams/reactive-streams-jvm#2.3

Co-authored-by: ikhoon ih.pert@gmail.com

Some of the subscribers violated Reactive Stremas 2.3 which is:
`Subscriber.onComplete()` and `Subscriber.onError(Throwable t)` MUST NOT call any methods on the `Subscription` or the `Publisher`.

https://github.com/reactive-streams/reactive-streams-jvm#2.3

Co-authored-by: ikhoon <ih.pert@gmail.com>
@minwoox minwoox added the defect label Jun 18, 2020
@minwoox minwoox added this to the 0.99.7 milestone Jun 18, 2020
@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #2815 into master will decrease coverage by 0.06%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2815      +/-   ##
============================================
- Coverage     72.78%   72.71%   -0.07%     
+ Complexity    11967    11951      -16     
============================================
  Files          1047     1047              
  Lines         46618    46616       -2     
  Branches       5829     5829              
============================================
- Hits          33929    33896      -33     
- Misses         9705     9727      +22     
- Partials       2984     2993       +9     
Impacted Files Coverage Δ Complexity Δ
...rmeria/internal/server/ResponseConversionUtil.java 85.71% <75.00%> (+1.71%) 11.00 <0.00> (ø)
...inecorp/armeria/server/HttpResponseSubscriber.java 80.31% <90.90%> (+0.39%) 60.00 <6.00> (+3.00)
...inecorp/armeria/common/ClosedSessionException.java 36.36% <0.00%> (-36.37%) 4.00% <0.00%> (-2.00%)
...necorp/armeria/internal/common/brave/SpanTags.java 66.66% <0.00%> (-33.34%) 3.00% <0.00%> (-2.00%)
...n/java/com/linecorp/armeria/server/HttpServer.java 40.00% <0.00%> (-20.00%) 2.00% <0.00%> (-1.00%)
...com/linecorp/armeria/client/brave/BraveClient.java 80.32% <0.00%> (-9.84%) 12.00% <0.00%> (-3.00%)
...r/rxjava2/ObservableResponseConverterFunction.java 83.33% <0.00%> (-5.56%) 15.00% <0.00%> (-1.00%)
...rmeria/internal/common/brave/TraceContextUtil.java 95.83% <0.00%> (-4.17%) 5.00% <0.00%> (ø%)
...com/linecorp/armeria/server/saml/SamlEndpoint.java 62.50% <0.00%> (-2.50%) 10.00% <0.00%> (-1.00%)
...om/linecorp/armeria/client/HttpSessionHandler.java 66.47% <0.00%> (-2.36%) 47.00% <0.00%> (-2.00%)
... and 19 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 1df220b...defa548. Read the comment docs.

@@ -227,7 +227,12 @@ public void onNext(T value) {
subscription.request(1);
});
} catch (Exception e) {
onError(e);
try {
writer.close(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Don't we need to check whether writer is open like we did in onError(...)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay just to call it because we checked it at line 214.

assert subscription != null;
subscription.cancel();
}
writer.close(cause);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try...catch and throwIfFatal(...)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @minwoox

Copy link
Collaborator

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @minwoox 😄

@trustin trustin merged commit 8ab8ca9 into line:master Jun 19, 2020
@minwoox minwoox deleted the fix_reactiveStream2.3 branch June 19, 2020 03:11
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Some of the subscribers violated Reactive Stremas 2.3 which is:
`Subscriber.onComplete()` and `Subscriber.onError(Throwable t)` MUST NOT call any methods on the `Subscription` or the `Publisher`.

https://github.com/reactive-streams/reactive-streams-jvm#2.3

Co-authored-by: ikhoon <ih.pert@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants