Improving ReactiveSocket abstraction.

[NiteshKant]

Today we have the following primary interfaces that application writers interact with:

ReactiveSocket

Primary abstraction for the multiplexed, full duplex connection following the protocol.

ConnectionSetupHandler

Used only at the server when a new connection is accepted.

RequestHandler

Used only at the server to handle various interaction models requested by the client.

Problem

There are a few issues/lack of feature with the above abstraction and current implementations:

• Client has no way of accepting requests from the server (this isn't currently implemented and I can imagine using ConnectionSetupHandler and RequestHandler for this purpose)
• RequestHandler and ReactiveSocket have very similar methods (primarily around various request interaction models) and hence is confusing as to why they are two different contracts.
• Client does not have a way to accept leases from the server. (this is also a lack of feature today but not impossible with the current abstractions)

Proposal

We can replace the 3 interfaces with two interfaces

ReactiveSocket

Keep the current interface but use it both as a way to request and respond to requests on a ReactiveSocket. This means for every connection there will be two ReactiveSocket instances on each end.

ReactiveSocketFactory (May be a better name)

On either end (client/server) there will be a factory which may look something like the below.

public interface ReactiveSocketFactory {

    /**
     * {@code ReactiveSocket} is a symmetric protocol where everything that a client can do can be done on the server
     * and vice-versa. So, every transport connection is associated with two {@code ReactiveSocket} instances, one for
     * responding to requests from the peer and another for sending requests to the peer.
     * This method is the converter that accepts a remote {@code ReactiveSocket} which is used to send requests to the
     * peer and returns a local {@code ReactiveSocket} that responds to requests from the peer.
     * 
     * @param requestingSocket Socket used to send request to the peer.
     * 
     * @return {@code ReactiveSocket} that is used to accept and respond to request from the peer.
     */
    ReactiveSocket accept(ReactiveSocket requestingSocket);
}

I am not yet sure, but this may look different at client than at the server due to the nature of replying to or sending a ConnectionSetupFrame. Anyways, this will be a replacement for ConnectionSetupHandler and either generify or provide for client side usecases.

This change will simplify the API a bit and also provide symmetric APIs for both client and server.

[benjchristensen]

Used only at the server to handle various interaction models requested by the client.

The terminology is a little off here. It is used on the "requester" side of the relationship. Since this protocol is bi-directional, it can be used on either client or server.

[benjchristensen]

Client has no way of accepting requests from the server

Well then that should be fixed. It used to be possible. A client had an overload that took a RequestHandler. Here is old code showing that: https://github.com/ReactiveSocket/reactivesocket-java/blob/v0.0.4/src/main/java/io/reactivesocket/ReactiveSocket.java#L110

[benjchristensen]

ReactiveSocket accept(ReactiveSocket requestingSocket);

I don't understand that signature. You return the same type as is passed in. Is it the same one? A different one?

It used to be very clear ... a server was created to accept connections, or a client connected to a server.

[NiteshKant]

There are two aspects to this change, let us discuss this one at a time.

Remove RequestHandler in favor of ReactiveSocket

RequestHandler and ReactiveSocket interfaces are identical when it comes to the different interaction models i.e.

    Publisher<Void> fireAndForget(final Payload payload);

    Publisher<Payload> requestResponse(final Payload payload);

    Publisher<Payload> requestStream(final Payload payload);

    Publisher<Payload> requestSubscription(final Payload payload);

    Publisher<Payload> requestChannel(final Publisher<Payload> payloads);

    Publisher<Void> metadataPush(final Payload payload);

So, one may think of RequestHandler as an implementation of ReactiveSocket.
(This of course is assuming that we do not have methods like onRequestReady, for which there is a separate issue.)
Thus, it appears to me that the presence of RequestHandler isn't really useful. Assuming that we don’t need it, ConnectionSetupHandler would look like:

public interface ConnectionSetupHandler {
    ReactiveSocket apply(ConnectionSetupPayload setupPayload, ReactiveSocket reactiveSocket) throws SetupException;
}

The above is what I was calling as ReactiveSocketFactory in my initial proposal, removing ConnectionSetupPayload which was a mistake.

Let us park the second part of the proposal related to ReactiveSocketFactory for a while and see if this change makes sense at first.

PS: Me and @stevegury were discussing offline that having start() method isn't also totally useful but we can save that for another issue.

[yschimke]

I really like this proposal for its elegance.

Does this allow two interesting use cases, or if not, what would be different?

1. a simple no serialization ReactiveSocket. This seems great for testing and mocking purposes. And the same RequestHandler.Builder pattern could be used for these ReactiveSockets.
2. trivial proxying of an existing ReactiveSocket over another transport. Especially interesting when a middle service can bridge two transports or networks.

[NiteshKant]

I really like this proposal for its elegance.
🎉 😄

I think the usability from a server end will roughly be the same just with less concepts (no RequestHandler). The benefit on the client side will be that it will be symmetric with the server, so again less concepts.

Both the usecases you mentioned I believe can be achieved today.

[yschimke]

I meant that today if I have a RequestHandler that implements my business logic, I can't use it anywhere that wants a ReactiveSocket without creating something like a Local Transport ReactiveSocket and going through serialization to frames etc. This is good and bad.

Likewise currently if I have a ReactiveSocket, to expose it over another transport I need to make a RequestHandler to wrap it.

[NiteshKant]

@yschimke aah .. correct! I didn't think about that but it is a nice consequence of this change 👍

[yschimke]

Talking more with @benjchristensen about it, a couple of points he brought up.

1. The signature for requestChannel is different deliberately, and allows the first payload to be peeked at for things like routing logic.
2. ReactiveSocket has additional methods that don't make sense when implementing a RequestHandler e.g. start, shutdown, on*

The cases I like can be pretty easily supported with adapters e.g. RequestHandler.fromOtherReactiveSocket(rs) and ReactiveSocket.buildLocal(requestHandler)

[yschimke]

Well then that should be fixed. It used to be possible. A client had an overload that took a RequestHandler. Here is old code showing that: https://github.com/ReactiveSocket/reactivesocket-java/blob/v0.0.4/src/main/java/io/reactivesocket/ReactiveSocket.java#L110

This isn't too bad, its just the simpler helper methods of DefaultRequestSocket

DefaultRequestSocket.fromClientConnection(DuplexConnection, ConnectionSetupPayload, RequestHandler, Consumer)
and
DefaultRequestSocket.fromClientConnection(DuplexConnection, ConnectionSetupPayload, Consumer)

I think real world usage will dictate whether we support 1-2 line connection building for both cases in each ReactiveSocketConnector impl, or you need to drop back to creating the DuplexConnection, passing it into fromClientConnection.

[NiteshKant]

Thanks for the input @benjchristensen @yschimke . Since @robertroeser and @stevegury are OOO I will wait for them to express their thoughts on this.

[benjchristensen]

Keep the current interface but use it both as a way to request and respond to requests on a ReactiveSocket. This means for every connection there will be two ReactiveSocket instances on each end.

I don't like this approach because it confuses the semantics of Requester and Responder.

Just because they happen to have Publisher for both input and output, that does not mean Requester and Responder are semantically the same. They are asymmetric in a few ways:

a) Developers will only ever implement the Responder interface, not the Requester, as the Requester APIs are always concretely provided by the reactivesocket-java library.

b) The Requester APIs are exposed to be invoked by developers, but Responder APIs are enveloped inside the state machine and invoked by reactivesocket-java upon receiving frames.

c) Practical usage of the interfaces resulted in the channel signature differing between Requester and Responder (not just name, but parameter signature) so that it provided an "initial payload". Without this it is difficult and costly to make routing decisions to which code a request should be handled with. Forcing Requester and Responder to be the same negatively affects usability.

I strongly thing that Requester and Responder should be treated differently, have different names, and allow their signatures to diverge as necessary, despite being similar. They are similar because they are the duals of each other, but they are not the same.

[benjchristensen]

• Client has no way of accepting requests from the server (this isn't currently implemented and I can imagine using ConnectionSetupHandler and RequestHandler for this purpose)
• Client does not have a way to accept leases from the server. (this is also a lack of feature today but not impossible with the current abstractions)

Both of these used to work and was quite elegant. Why not just revert to the prior design? What was the reason for moving away from it?

[NiteshKant]

Just because they happen to have Publisher for both input and output, that does not mean Requester and Responder are semantically the same.

That very clearly is not the reason for this change. The reason is that they both deal with same interactions (FnF, Request-Response, ....), one requests and another responds with same signature. To me separating out the first Payload for requestChannel is not convincing enough reason for them to have different interfaces, YMMV.

[xytosis]

About separating out the first Payload for requestChannel, I would like to be able to access the Payload before subscribing. Clients may want to engage different channel behaviors, and should be able to specify it cleanly; I am running into this problem while writing the TCK. Right now, I have to define a handler that has to call request(n) and then decide what response to give based on the first payload received, which does not seem as elegant as it should.

[NiteshKant]

@xytosis I don't think I follow you completely.

Are you referring to the stream of payloads returned from this method in ReactiveSocket:

Publisher<Payload> requestChannel(final Publisher<Payload> payloads);

and you want RequestHandler kind of functionality that gives you the first payload and then the remaining stream (although RequestHandler stream includes the first Payload)?

[xytosis]

@NiteshKant
Yes, I think it would make for cleaner handler code if we could get the first payload in channel like we can in requestStream/Subscription/Response.

[NiteshKant]

Ok so you are suggesting that this method in ReactiveSocket

Publisher<Payload> requestChannel(final Publisher<Payload> payloads);

can be changed to

Publisher<Payload> requestChannel(final Payload first, final Publisher<Payload> payloads);

It is interesting because a channel is symmetric and the need to peek to the first payload while sending a request may also apply to the received payload stream (returned Publisher here).

I wonder if this can be achieved by providing a utility like:

public static <T> void peekFirst(Publisher<T> source, BiConsumer<T, Publisher<T>> consumer)

which basically removes the need of this peculiarity in the main interface.

[benjchristensen]

(I believe) That is inefficient as it requires either subscribing twice, or multicasting.

[benjchristensen]

That very clearly is not the reason for this change.

But that's what this change looks to be pursuing, as I see no benefit of ever having the same interface shared for both. No developer is ever implementing the Requester side, only the Responder side.

I am really struggling to see the real benefits of this change, only negatives.