Skip to content

Subscriptions onComplete drops message#1781

Closed
ashley-taylor wants to merge 2 commits into
graphql-java:masterfrom
ashley-taylor:dropped-completion-stage-message
Closed

Subscriptions onComplete drops message#1781
ashley-taylor wants to merge 2 commits into
graphql-java:masterfrom
ashley-taylor:dropped-completion-stage-message

Conversation

@ashley-taylor
Copy link
Copy Markdown

When onComplete is called from the parent subscription it instantly
completes the downstream subscriptions.
If any messages are still being processed in a CompletionStage they
where lost.
This patch confirms no messages are in flight before completing
downstream endpoint

Ashley Taylor added 2 commits February 3, 2020 15:53
When onComplete is called from the parent subscription it instantly
completes the downstream subscriptions.
If any messages are still being processed in a CompletionStage they
where lost.
This patch confirms no messages are in flight before completing
downstream endpoint
@ashley-taylor
Copy link
Copy Markdown
Author

The Unit test that fails also fails on master for me.
Is there a configuration setting or something I am missing.


when:
Publisher<Integer> rxIntegers = Flowable.range(0, 10)
Executor executor = CompletableFuture.delayedExecutor(50, TimeUnit.MILLISECONDS)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The tests are failing because CompletableFuture.delayedExecutor is a Java 9 API

@bbakerman
Copy link
Copy Markdown
Member

Closing this in favour of the other PR

However thanks for this PR because it helped us progress the problem and helped us attack it in code terms

@bbakerman bbakerman closed this Mar 9, 2020
@ashley-taylor
Copy link
Copy Markdown
Author

Awesome, missed your comment from last week.
Thanks for fixing it.

@ashley-taylor ashley-taylor deleted the dropped-completion-stage-message branch March 9, 2020 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants