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

Fix concat map request race condition which resulted in flaky test #1341

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

ozangunalp
Copy link
Collaborator

Change how the upstream request is deferred until the next downstream request

Change how the upstream request is deferred until the next downstream request
@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Merging #1341 (6f5500e) into main (dfe08a5) will increase coverage by 0.30%.
The diff coverage is 72.72%.

❗ Current head 6f5500e differs from pull request most recent head a81c239. Consider uploading reports for the commit a81c239 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1341      +/-   ##
============================================
+ Coverage     89.64%   89.95%   +0.30%     
- Complexity     3349     3361      +12     
============================================
  Files           459      459              
  Lines         13327    13325       -2     
  Branches       1656     1655       -1     
============================================
+ Hits          11947    11986      +39     
+ Misses          711      694      -17     
+ Partials        669      645      -24     
Files Changed Coverage Δ
...llrye/mutiny/operators/multi/MultiConcatMapOp.java 80.00% <72.72%> (+6.77%) ⬆️

... and 17 files with indirect coverage changes

@cescoffier cescoffier requested a review from jponge August 17, 2023 10:40
@jponge jponge added the bug Something isn't working label Aug 22, 2023
@jponge jponge added this to the 2.4.0 milestone Aug 22, 2023
@@ -70,9 +68,9 @@ void testTransformToMulti(boolean prefetch, int[] upstreamRequests) {
Multi<Integer> result = upstream.onItem()
.transformToMulti(i -> Multi.createFrom().items(i, i))
.concatenate(prefetch);
AssertSubscriber<Integer> ts = new AssertSubscriber<>(5);
AssertSubscriber<Integer> ts = new AssertSubscriber<>();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for digging into that bug @ozangunalp 👍

Minor question: why did you update the test case here? The split 5 upfront then 5 requests scheme had a race condition fixed before, but why not keep it in this test, still?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ts.awaitSubscription(); in the other test reproduced the test case more consistently.
For this one, during bug fix tests, I had a dropped item, even with this change. So I kept it like that.

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks!

@jponge jponge linked an issue Aug 22, 2023 that may be closed by this pull request
@jponge jponge merged commit e6363bd into smallrye:main Aug 24, 2023
@jponge jponge modified the milestones: 2.4.0, 2.4.0-RC1 Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate why MultiConcatMapNoPrefetchTest is a flaky test
2 participants