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 potential nontermination in Observable.zip[Map]3 #592

Merged
merged 2 commits into from Feb 15, 2018

Conversation

Projects
None yet
2 participants
@oleg-py
Collaborator

oleg-py commented Feb 15, 2018

Fixes #590

This needs to be replicated for other zipMapX operators, but I want some feedback first :)

The behavior expected for zipped observable is to complete as soon as any single of zipped stream completes, which is totally not what current implementation does. Before this fix, it would complete only if one of streams called onComplete not concurrently with onNext or, if neither did, when all streams called onComplete. Waiting for all streams would result on non-termination when backpressuring, as a zipped one would never emit its next result, or would attempt to resolve same promise twice, concurrently.

Tests didn't catch this because they only use equally sized observables.

@alexandru

This comment has been minimized.

Member

alexandru commented Feb 15, 2018

Ah shit, that's not good. We need a backport to 2.x as well 🙁

@codecov

This comment has been minimized.

codecov bot commented Feb 15, 2018

Codecov Report

Merging #592 into master will decrease coverage by 6.6%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #592      +/-   ##
==========================================
- Coverage   90.68%   84.07%   -6.61%     
==========================================
  Files         373      373              
  Lines        9904    10697     +793     
  Branches     1849     1952     +103     
==========================================
+ Hits         8981     8994      +13     
- Misses        923     1703     +780

@alexandru alexandru merged commit d767afb into monix:master Feb 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexandru

This comment has been minimized.

Member

alexandru commented Feb 15, 2018

Could you also do a backport on the series/2.x branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment