reactive-ipc / reactive-ipc-jvm

Reactive IPC for the JVM
Apache License 2.0
55 stars 13 forks source link

Server Handler Return Type #26

Open benjchristensen opened 9 years ago

benjchristensen commented 9 years ago

Server handlers can be implemented with void as their return type. I suggest however that we should return something else, such as Publisher<Void>. Reasons are as follows:

1) Error Handling

By returning Publisher<Void> the server can capture errors and then do whatever is appropriate:

If it returns void all that can be done is throw an exception or manually log an error.

Additionally, throwing exceptions from the handler is not ideal since it is often doing async work so the error will be thrown to whatever is invoking a callback (via onNext).

2) Logging

The lifecycle of the handler can easily bbe logged and metrics captured if a Publisher<Void> is returned since it will subscribe to it to start the handler and then can capture the onComplete and onError terminal events.

This can indirectly be done by intercepting the input/output streams, but there isn't a formal definition of "completion" on an output stream which leads to the next point ...

3) Completion

If a handler returns void, when does the connection get closed? It would require an imperative method such as connection.close() or requires the user to manage the subscription and manually unsubscribe ...

4) User Subscribes

... and through a couple years experience with RxJava I have found it to be a significant anti-pattern if a user ever needs to manually subscribe to a Publisher. The developer should declare the lazy code, return the Publisher and let the server manage the lifecycle of subscribing. By emitting onComplete the developer tells the server to terminate gracefully, or emitting onError it shuts down.

Examples will be provided below.

benjchristensen commented 9 years ago

ping @NiteshKant for examples

NiteshKant commented 9 years ago

I am giving examples assuming we agree on #22 and the write signature is:

Publisher<Void> write(Publisher<T> data);

This also assumes that since write returns a Publisher, the execution is lazy and is done onSubscribe i.e. the data does not get written on the connection, till the returned Publisher<Void> is subscribed.

Now, if the ConnectionHandler is defined as (PR #20 here)

public interface Handler<T> {
    /**
     * Handle the given object (do something useful with it).
     * @param obj
     */
    void handle(T obj);
}

An implementation that just writes "Hello World" would look like:

        /// Option 1: No logging, error handling, connection close, etc.
        connection.write(Observable.just("Hello World"))
                  .subscribe(...); // No Error Handling

        /// Option 2: With logging/metric
        connection.write(Observable.just("Hello World"))
                  .doOnSubscribe(/*Log processing start*/)
                  .doOnError(/*Log processing error*/)
                  .doOnComplete(/*Log processing complete*/)
                  .subscribe(...); 

        /// Option 2: With connection close
        connection.write(Observable.just("Hello World"))
                  .concatWith(connection.close())
                  .doOnError()
                  .doOnComplete()
                  .subscribe(...); 

where as, if we return a Publisher

public interface Handler<T> {
    /**
     * Handle the given object (do something useful with it).
     * @param obj
     */
    Publisher<Void> handle(T obj);
}

and have the caller of handle() do the error, logging and subscription, the code will simply be:

        connection.write(Observable.just("Hello World"));

This of course is assuming that we need metrics, connection close done by the library and not user code. Which is preferred as it is mostly boilerplate code.

NiteshKant commented 9 years ago

PR #19 defines a server handler as

public interface TcpHandler<R, W> {

    Publisher<Void> handle(TcpConnection<R, W> connection);
}

If there are no concerns around this definition, may I close this issue?