Skip to content

Conversation

stevegury
Copy link
Member

Problem
Instead of manually requesting in the onSubscribe, it's better
to actually pass the Subscription to the Subscriber.
The *Connector code was a little bit confusing because the Subscriber
like the Subscription were names s.

Solution
Rename the Subscriber -> subscriber, and pass the subscription.

**Problem**
Instead of manually requesting in the `onSubscribe`, it's better
to actually pass the `Subscription` to the `Subscriber`.
The *Connector code was a little bit confusing because the `Subscriber`
like the `Subscription` were names `s`.

**Solution**
Rename the `Subscriber` -> `subscriber`, and pass the subscription.
@NiteshKant
Copy link
Contributor

Ok so this is working today because no where in the code we are actually validating backpressure for this invocation i.e. if we receive an onNext without actually requesting 😞
So, it is the correct thing to do but doesn't really make a difference today, rite?

@stevegury
Copy link
Member Author

The main problem here, is that we're requesting 1 DuplexConnection without ensuring that the Publisher chain of the ReactiveSocket is initialized. We were breaking the chain by introducing a request(1).

For instance, this fixes the problem when you receive an onError without having an onNext which could fail due to the lack of initialization.

@NiteshKant
Copy link
Contributor

NiteshKant commented Jun 22, 2016

Yes I understood that. My surprise was "why was it working till now?" :) Which I think is because we do not do this sanity check: "Did I get an onNext before onSubscribe()?"

Anyways, this change looks good, merging..

@NiteshKant NiteshKant merged commit 59a4ebe into master Jun 22, 2016
@stevegury stevegury deleted the stevegury/fix-connector branch July 15, 2016 22:32
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