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

Issue 352: Change SubscriberAsReactiveSubscriber to properly apply back-pressure #361

Merged
merged 7 commits into from
May 3, 2017

Conversation

alexandru
Copy link
Member

The SubscriberAsReactiveSubscriber conversion does not work properly for async Subscribers, because in version 2.2.4 it uses an unbounded buffer and it keeps requesting from the source without listening to the Future[Ack] given by the downstream subscriber. The communication in this case is OK, since a buffer is being used, but the source is not paused while the target is busy.

This PR fixes the implementation to apply back-pressure using when communicating with the Reactive Streams Publisher, in accordance with the response given by the downstream Monix Subscriber.

Other changes:

  • add TrampolinedExecutionContext.immediate reference, used as an optimization
  • adds tests for this back-pressure change to guard against regressions
  • Observable.fromReactivePublisher gets an overload that can specify the requestCount on each batch
  • the bufferSize param in that call chain gets renamed to requestCount, because the actual buffer in the implementation remains Unbounded (yet its size will be controlled by requestCount due to back-pressure)

/cc @jchapuis

@codecov
Copy link

codecov bot commented May 3, 2017

Codecov Report

Merging #361 into master will decrease coverage by 0.03%.
The diff coverage is 66%.

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   86.32%   86.28%   -0.04%     
==========================================
  Files         313      313              
  Lines        8378     8409      +31     
  Branches     1662     1679      +17     
==========================================
+ Hits         7232     7256      +24     
- Misses       1146     1153       +7

@codecov
Copy link

codecov bot commented May 3, 2017

Codecov Report

Merging #361 into master will increase coverage by 0.02%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   86.32%   86.35%   +0.02%     
==========================================
  Files         313      313              
  Lines        8378     8411      +33     
  Branches     1662     1664       +2     
==========================================
+ Hits         7232     7263      +31     
- Misses       1146     1148       +2

@alexandru alexandru merged commit e151ad9 into master May 3, 2017
Copy link

@jchapuis jchapuis left a comment

Choose a reason for hiding this comment

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

great! I can see this triggered quite a good number of changes. Also the additional coverage looks awesome, since effects of backpressure and cancelation are hard to observe from the outside.

@alexandru alexandru deleted the issue-352 branch May 4, 2017 05:27
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