Skip to content

Commit

Permalink
Refactor RequestHandler.handleChannel to only take a Publisher<Payloa…
Browse files Browse the repository at this point in the history
…d> as argument.

The `initialPayload` was already part of the `payloads` publisher, this was very confusing.
+ Fix the tests impacted by the new API.
+ Code style: Ensure lines length < 100 characters
  • Loading branch information
stevegury committed Oct 26, 2015
1 parent 468dadf commit adb7ce5
Show file tree
Hide file tree
Showing 13 changed files with 1,852 additions and 1,511 deletions.
8 changes: 7 additions & 1 deletion src/main/java/io/reactivesocket/DuplexConnection.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,11 @@ public interface DuplexConnection extends Closeable {
Observable<Frame> getInput();

void addOutput(Publisher<Frame> o, Completable callback);


default void addOutput(Frame frame, Completable callback) {
addOutput(s -> {
s.onNext(frame);
s.onComplete();
}, callback);
}
}
Loading

4 comments on commit adb7ce5

@benjchristensen
Copy link
Contributor

Choose a reason for hiding this comment

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

We very consciously separated out the initialPayload. Why are you reverting that? It makes routing decisions on the initialPayload more expensive and difficult.

@benjchristensen
Copy link
Contributor

Choose a reason for hiding this comment

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

The unit test proving the issue was refactored to break the point of what was being tested. It no longer routes to the 'echo' method was it was suppose to. Now to do routing decisions on the initial frame the stream must be multicasted so both the router, and the method can subscribe to it. This is much more complicated logic for the standard case of routing based on the initial frame. Routing is almost always needed in applications.

@stevegury
Copy link
Member Author

Choose a reason for hiding this comment

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

The motivation behind this change was to simplify the implementation of a RequestHandler (from a user point of view). I was trying simplify the non-routing logic.
I didn't have the context behind the decision of introducing the initialRequest, it all makes sense now.
I'll refactor.

@benjchristensen
Copy link
Contributor

Choose a reason for hiding this comment

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

@stevegury cool. Glad it makes sense. I should have put more information in the javadocs!

Please sign in to comment.