Skip to content

Subscriber cleanup#137

Merged
stevegury merged 2 commits intorsocket:masterfrom
NiteshKant:subscriber-cleanup
Jul 11, 2016
Merged

Subscriber cleanup#137
stevegury merged 2 commits intorsocket:masterfrom
NiteshKant:subscriber-cleanup

Conversation

@NiteshKant
Copy link
Copy Markdown
Contributor

@NiteshKant NiteshKant commented Jul 10, 2016

Problem

Publishers was using different implementations of CancellableSubscriber to provide different callbacks using inheritance. eg: by providing methods like doOnNext, doOnCancel, etc.

This is problematic as it requires calls to super.do* by implementator. Also, it created unnecessary class hierarchy with little benefit.

(Edit: Adding more context from the comments below)

There were 3 classes CancellableSubscriber, SafeCancellableSubscriber and SafeCancellableSubscriberProxy, each of them would override some of the methods from Subscriber. eg: doOnCancel was something that was to be used by SafeCancellableSubscriber and CancellableSubscriber in a way that SafeCancellableSubscriber had to implement doOnCancel and provide yet another method doOnCancel0 which if implemented by SafeCancellableSubscriberProxy will have to provide yet another protected method.
So, I figured not only the approach was error prone it was painful to code and maintain. Thus this approach where in specific callbacks can be overridden and controlled by the caller.

Modification

Rolled up all implementations into a single class that takes various callbacks as different lambdas.
This is easier to use and there are less chances of errors when the implementor forgot to call super methods.

Thanks @yschimke for the comment in PR #127 which led me to change the approach.

#### Problem

`Publishers` was using different implementations of `CancellableSubscriber` to provide different callbacks using inheritance. eg: by providing methods like `doOnNext`, `doOnCancel`, etc.

This is problematic as it requires calls to `super.do*` by implementator. Also, it created unnecessary class hierarchy with little benefit.

#### Modification

Rolled up all implementations into a single class that takes various callbacks as different lambdas.
This is easier to use and there are less chances of errors when the implementor forgot to call `super` methods.

Thanks @yschimke for the comment in PR rsocket#127 which led me to change the approach.
@yschimke
Copy link
Copy Markdown
Member

I'm not sure my comment meant that changing to a deep Java 8 approach was the only solution, I suspect that more private methods and choosing which public doXXX to override was possible.

But I'm interested at the same time to see how this plays out. LGTM

@NiteshKant
Copy link
Copy Markdown
Contributor Author

@yschimke Indeed your comment didn't suggest this approach but made me realize the problem with the older approach. Here was the problem with the older approach:

There were 3 classes CancellableSubscriber, SafeCancellableSubscriber and SafeCancellableSubscriberProxy, each of them would override some of the methods from Subscriber. eg: doOnCancel was something that was to be used by SafeCancellableSubscriber and CancellableSubscriber in a way that SafeCancellableSubscriber had to implement doOnCancel and provide yet another method doOnCancel0 which if implemented by SafeCancellableSubscriberProxy will have to provide yet another protected method.
So, I figured not only the approach was error prone it was painful to code and maintain. Thus this approach where in specific callbacks can be overridden and controlled by the caller.

Instead of sending an error, on detecting duplicate `onSubscribe` call, according to the spec, we should send a `cancel`
@stevegury
Copy link
Copy Markdown
Member

stevegury commented Jul 11, 2016

I had the same knee-jerk reaction than @yschimke but your comment motivate the current implementation.
Could you update the description of the PR so that the commit message contains the information?

Otherwise, 👍

@NiteshKant
Copy link
Copy Markdown
Contributor Author

@stevegury updated the PR description

@stevegury stevegury merged commit 67e42f4 into rsocket:master Jul 11, 2016
@NiteshKant NiteshKant deleted the subscriber-cleanup branch July 12, 2016 22:40
ilayaperumalg pushed a commit to ilayaperumalg/rsocket-java that referenced this pull request Dec 26, 2017
* Add RESPONSE Next Flag

As per discussion in rsocket/rsocket#126 in order to remove ambiguity and use of sentinel value of 0-bytes, and allow 0-byte payloads.

* Update Protocol.md

* Clarify Data or Metadata for Next.

* Clarify the sample code
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.

3 participants