Issue #11803 - Follow Reactive Streams specification #11804

scrat98 · 2024-05-16T14:24:28Z

The main improvements:

safely close resources in case of any errors to avoid resource leaks
multi-threaded reading from Content.Source is avoided
more accurate implementation of Reactive Streams and passed TCK tests

jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/content/ContentSourcePublisherTest.java

scrat98 · 2024-05-16T21:19:56Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

+ // As per rule 3.13, Subscription.cancel() MUST request the Publisher to eventually
+ // drop any references to the corresponding subscriber.
+ this.demand.set(NO_MORE_DEMAND);
+ // TODO: HttpChannelState does not satisfy the contract of Content.Source "If read() has returned a last chunk, this is a no operation."


HttpChannelState does not satisfy the contract of Content.Source.fail method "If read() has returned a last chunk, this is a no operation."

scrat98 · 2024-05-17T10:30:38Z

jetty-core/jetty-io/pom.xml

@@ -50,6 +55,22 @@
 <argLine>@{argLine} ${jetty.surefire.argLine} 
 --add-reads org.eclipse.jetty.io=org.eclipse.jetty.logging</argLine>
 </configuration>
+ <dependencies>
+ <!--


It's ugly, but maybe I could:

Create pull request to the reactive streams TCK and support also junit platform

Have a look at the surefire plugin and try to understand if it possible to make junit5 with testng working together out of the box

yup no much choice.

It would be good to add parsing of testng result files for Jenkins.

olamy@pop-os:~/dev/sources/jetty/jetty.project$ ls -lrt jetty-core/jetty-io/target/surefire-reports/Surefire\ suite/ total 72 -rw-rw-r-- 1 olamy olamy 60957 May 22 12:43 'Surefire test.html' -rw-rw-r-- 1 olamy olamy 7951 May 22 12:43 'Surefire test.xml' -rw-rw-r-- 1 olamy olamy 2127 May 22 12:43 testng-failed.xml

See the Jenkinsfille
transform

junit testResults: '**/target/surefire-reports/*.xml,**/target/invoker-reports/TEST*.xml', allowEmptyResults: true

into

junit testResults: '**/target/surefire-reports/**/*.xml,**/target/invoker-reports/TEST*.xml', allowEmptyResults: true

This should make it (at least we will see :))

@olamy if you have time, could you please update jenkins configuration, because I'm not so much familiar with it

I cannot push to your fork/branch. So you will need to cherry pick this commit 5f0168b

cherry-picked, thanks. Jenkins could found the tests reports https://jenkins.webtide.net/job/jetty.project/job/PR-11804/20/testReport/org.eclipse.jetty.io.content/ContentSourcePublisherTest/

Is this new testng requirement because it's used by import import org.reactivestreams.tck.TestEnvironment; ?

@joakime unfortunately yes https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck

I'm not a fun to mix multiple testing frameworks, but it is what it is

gregw

Thanks for this... looks good, but will need careful review. Just a couple of trivial comments first whilst I find time to look deeper....

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/content/ContentSourcePublisherTest.java

gregw

More feedback...

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

pom.xml

olamy · 2024-05-22T02:50:36Z

jetty-core/jetty-io/pom.xml

@@ -50,6 +55,22 @@
 <argLine>@{argLine} ${jetty.surefire.argLine} 
 --add-reads org.eclipse.jetty.io=org.eclipse.jetty.logging</argLine>
 </configuration>
+ <dependencies>
+ <!--


It would be good to add parsing of testng result files for Jenkins.

olamy@pop-os:~/dev/sources/jetty/jetty.project$ ls -lrt jetty-core/jetty-io/target/surefire-reports/Surefire\ suite/ total 72 -rw-rw-r-- 1 olamy olamy 60957 May 22 12:43 'Surefire test.html' -rw-rw-r-- 1 olamy olamy 7951 May 22 12:43 'Surefire test.xml' -rw-rw-r-- 1 olamy olamy 2127 May 22 12:43 testng-failed.xml

See the Jenkinsfille
transform

junit testResults: '**/target/surefire-reports/*.xml,**/target/invoker-reports/TEST*.xml', allowEmptyResults: true

into

junit testResults: '**/target/surefire-reports/**/*.xml,**/target/invoker-reports/TEST*.xml', allowEmptyResults: true

This should make it (at least we will see :))

lorban · 2024-05-22T07:33:52Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java


 public ContentSourcePublisher(Content.Source content)
 {
- this.content = content;
+ if (content == null)


Better use Objects.requireNonNull(content)

@lorban thank you, fixed

lorban · 2024-05-22T07:35:28Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

+ // As per rule 2.13, we MUST consider subscription cancelled and
+ // MUST raise this error condition in a fashion that is adequate for the runtime environment.
+ subscription.cancel(err, LastWillSubscription.FinalSignal.SUPPRESS);
+ LOG.error("Flow.Subscriber " + subscriber + " violated rule 2.13", err);


Wouldn't it be better to rethrow rather than logging?

@lorban According to the specification we cannot throw any exception

As per rule 1.9, subscribe method must return normally (i.e. not throw)

As per rule 2.13, ...the caller MUST raise this error condition in a fashion that is adequate for the runtime environment. «Raise this error condition in a fashion that is adequate for the runtime environment» could mean logging the error—or otherwise make someone or something aware of the situation—as the error cannot be signalled to the faulty Subscriber.

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

lorban · 2024-05-22T07:38:35Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

- stalled = false;
- process = true;
- }
+ if (reason == null)


Better use Objects.requireNonNull()

@lorban thank you, fixed

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

lorban · 2024-05-22T07:41:15Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

- content.fail(chunk.getFailure());
- subscriber.onError(chunk.getFailure());
- return;
+ LOG.error("Flow.Subscriber " + subscriber + " violated rule 2.13", err);


Why are you catching Throwable here?

@lorban ActiveSubscription.process() method triggered from Subscription.cancel/request methods and may be executed synchronously

3.10, 3.11 While the Subscription is not cancelled, Subscription.request(long n) MAY synchronously call onNext on this

3.15 and 3.16 Calling Subscription.cancel/request MUST return normal (that's the reason why we cannot throw here)

2.13, ...the caller MUST raise this error condition in a fashion that is adequate for the runtime environment. «Raise this error condition in a fashion that is adequate for the runtime environment» could mean logging the error—or otherwise make someone or something aware of the situation—as the error cannot be signalled to the faulty Subscriber.

lorban · 2024-05-22T07:41:45Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java

+ catch (Throwable err)
+ {
+ cancel(err, FinalSignal.SUPPRESS);
+ LOG.error("Flow.Subscriber " + subscriber + " violated rule 2.13", err);


Why not rethrowing?

@lorban the same reason as above

…eactive streams tck Signed-off-by: Olivier Lamy <[email protected]>

Simplified #11804 + removed LastWillSubscription, LastWill and FinalSignal + introduced Suppressed and Cancelled Throwables + updated notes

gregw · 2024-05-28T01:01:55Z

@scrat98 I thought you might be getting a bit tired of our exacting review process.... so I branched this in #11849 and changed it to how I'd like to see it. The key commit c216b00, which removes all the LastWill stuff, which I don't like for the name and for the additional complexity. Instead, I'm just using typed Throwables to signal the type of cancellation.

It passes the test.

Feel free to bring in that commit here if you wish to make further modifications and I'll close the other PR.

Simplified #11804 + removed LastWillSubscription, LastWill and FinalSignal + introduced Suppressed and Cancelled Throwables + updated notes

scrat98 · 2024-05-30T08:08:29Z

@scrat98 I thought you might be getting a bit tired of our exacting review process.... so I branched this in #11849 and changed it to how I'd like to see it. The key commit c216b00, which removes all the LastWill stuff, which I don't like for the name and for the additional complexity. Instead, I'm just using typed Throwables to signal the type of cancellation.

It passes the test.

Feel free to bring in that commit here if you wish to make further modifications and I'll close the other PR.

No worries, I'm much appreciated for such review process :) Thank you a lot for your time, I'll have a look at your changes this weekends(I'm sorry don't have enough free time this week) and cherry pick them. But the general idea is clear, thanks again

gregw

I think this looks good to go. Would be good to get it merged early in a release cycle.
I think the CI failures are unrelated, but we should check

scrat98 · 2024-06-05T00:06:18Z

I think this looks good to go. Would be good to get it merged early in a release cycle. I think the CI failures are unrelated, but we should check

Hi @gregw. If you don't mind let me day tomorrow, I would like to consider your changes as well and remove some todos from the code. I know that promised do it 2 days ago, but only tomorrow finally will have spare day to finish this merge request. Thanks in advance

scrat98 · 2024-06-05T23:52:41Z

@gregw I think I'm done, one more time reviewed every comment and all changes. Small refactoring:

made LastWill COMPLETED static
mentioned about HttpChannelState issue. I tried to solve it, but seems I need more time to understand the code base
used fast path when this.subscriber.onNext(chunk) failed

About your suggested changes:

"LastWill" naming

I borrowed that term from the mqtt protocol and I thought that's well known design pattern
It's not publicly available, but only private utility class. Therefore I think the chance that open source community will create holivars off-topics about it is very low

"LastWill" abstraction

It should help think less while implementing RS specification. For example, you don't need to think if it Exhausted or Active subscription. The rule of thumb that subscription must always be cancelled. Also I like explicitly specify final state, it seems more natural look at all possible states in the enum rather than using instanceof and thinking about all possible subclasses.
It avoids creating wrapper of the Exception(like for suppressed exceptions) and just uses original exception as it is.

Nevertheless, I gave you full access to the fork, feel free to merge your changes, I'm absolutely okay with it.

About CI/CD tests it seems there are fluky tests, but this failed test I found really strange https://jenkins.webtide.net/job/jetty.project/job/PR-11804/20/testReport/org.eclipse.jetty.server/LoggerDebugTest/Parallel_Stage___Build___Test___JDK22___testWrapperNPE/

gregw · 2024-06-06T03:34:18Z

@scrat98 I have reopened by simplification of this in #11849. Not only do I not like the LastWill name, I don't think we even need that abstraction. I think my branch is meeting all the tests without it.
So my intention is to go forward with merging that PR once it is reviewed by others (you will still get author credit as it has your commits in the PR). I can't see anything in your latest changes that needs to be copied over... but please do check that branch.

thanks for this contribution.

Simplified jetty#11804 + removed LastWillSubscription, LastWill and FinalSignal + introduced Suppressed and Cancelled Throwables + updated notes

scrat98 · 2024-06-06T23:09:35Z

@gregw I prefer to merge your changes to this PR for the following reasons:

Keep tracking all comments in this PR
Preserve original git history
Keep changes about handling failure of this.subscriber.onNext(chunk) more properly and also comment about HttpChannelState issue
I think Cancellation exception not needed as it not used

So I have cherry-picked your commit c216b00. If it's okay, please merge scrat98#1

Issue jetty#11803 - Follow Reactive Streams specification

f2dbc2d

scrat98 commented May 16, 2024

View reviewed changes

jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/content/ContentSourcePublisherTest.java Outdated Show resolved Hide resolved

scrat98 commented May 16, 2024

View reviewed changes

jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/content/ContentSourcePublisherTest.java Show resolved Hide resolved

scrat98 added 2 commits May 16, 2024 19:08

Issue jetty#11803 - upgrade testng version

5c25536

Issue jetty#11803 - workaround for HttpChannelState

9e9ed21

scrat98 commented May 16, 2024

View reviewed changes

Issue jetty#11803 - support testng with maven-surefire-plugin

b88ae63

scrat98 commented May 17, 2024

View reviewed changes

Merge branch 'jetty:jetty-12.0.x' into jetty-12.0.x

2002f0f

gregw requested changes May 20, 2024

View reviewed changes

scrat98 added 2 commits May 21, 2024 22:01

Issue jetty#11803 - code review refactoring

84b0c61

Merge branch 'jetty:jetty-12.0.x' into jetty-12.0.x

8170039

gregw requested review from sbordet and lorban May 21, 2024 22:14

gregw requested changes May 21, 2024

View reviewed changes

olamy requested changes May 22, 2024

View reviewed changes

lorban requested changes May 22, 2024

View reviewed changes

gregw requested review from gregw, lorban and olamy May 23, 2024 07:20

scrat98 and others added 7 commits May 25, 2024 09:06

Issue jetty#11803 - use Objects.requireNonNull

7e1a83d

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x

b118cf9

Issue jetty#11803 - remove unused reactive-streams deps

f410849

Merge branch 'jetty:jetty-12.0.x' into jetty-12.0.x

f2d14bc

Issue jetty#11803 - use static exception for normal completion

b60fca3

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x

46dc7b3

adapt Jenkins junit parser to take into account testng results from r…

5f0168b

…eactive streams tck Signed-off-by: Olivier Lamy <[email protected]>

gregw added a commit that referenced this pull request May 28, 2024

Simplification of #11804 for Reactive Stream specification support

c216b00

Simplified #11804 + removed LastWillSubscription, LastWill and FinalSignal + introduced Suppressed and Cancelled Throwables + updated notes

gregw mentioned this pull request May 28, 2024

Simplified Reactive Stream update for ContentSourcePublisher #11849

Open

olamy pushed a commit that referenced this pull request May 29, 2024

Simplification of #11804 for Reactive Stream specification support

0949841

Simplified #11804 + removed LastWillSubscription, LastWill and FinalSignal + introduced Suppressed and Cancelled Throwables + updated notes

scrat98 force-pushed the jetty-12.0.x branch from e933095 to 5f0168b Compare June 4, 2024 16:04

Merge branch 'jetty:jetty-12.0.x' into jetty-12.0.x

c05d32c

gregw previously approved these changes Jun 4, 2024

View reviewed changes

Issue jetty#11803 - SyntheticContentSource: replace todo with note

5a54e00

scrat98 dismissed gregw’s stale review via 5a54e00 June 5, 2024 19:24

Issue jetty#11803 - refactoring

524988c

scrat98 mentioned this pull request Jun 5, 2024

HttpChannelState does not satisfy the contract of Content.Source #11879

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #11803 - Follow Reactive Streams specification #11804

Issue #11803 - Follow Reactive Streams specification #11804

scrat98 commented May 16, 2024

scrat98 May 16, 2024 •

edited

scrat98 Jun 5, 2024

scrat98 May 17, 2024

gregw May 20, 2024

olamy May 22, 2024

olamy May 22, 2024

scrat98 May 25, 2024

olamy May 27, 2024

scrat98 Jun 4, 2024 •

edited

joakime Jun 4, 2024

scrat98 Jun 4, 2024

gregw left a comment

gregw left a comment

olamy May 22, 2024

lorban May 22, 2024

scrat98 May 25, 2024

lorban May 22, 2024

scrat98 May 25, 2024

lorban May 22, 2024

scrat98 May 25, 2024

lorban May 22, 2024

scrat98 May 26, 2024

lorban May 22, 2024

scrat98 May 26, 2024

gregw commented May 28, 2024

scrat98 commented May 30, 2024

gregw left a comment

scrat98 commented Jun 5, 2024

scrat98 commented Jun 5, 2024 •

edited

gregw commented Jun 6, 2024

scrat98 commented Jun 6, 2024

Issue #11803 - Follow Reactive Streams specification #11804

Are you sure you want to change the base?

Issue #11803 - Follow Reactive Streams specification #11804

Conversation

scrat98 commented May 16, 2024

scrat98 May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrat98 Jun 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented May 28, 2024

scrat98 commented May 30, 2024

gregw left a comment

Choose a reason for hiding this comment

scrat98 commented Jun 5, 2024

scrat98 commented Jun 5, 2024 • edited

gregw commented Jun 6, 2024

scrat98 commented Jun 6, 2024

scrat98 May 16, 2024 •

edited

scrat98 Jun 4, 2024 •

edited

scrat98 commented Jun 5, 2024 •

edited