Hi Pavel,

On 9/2/2015 4:40 PM, Pavel Rappo wrote:
Hi Roger, thanks for looking at this!

On 1 Sep 2015, at 17:05, Roger Riggs <roger.ri...@oracle.com> wrote:

- The Incoming class combines separate functions that would be easier to
  use if the methods were directly on the Builder.
...

|WebSocket ws = WebSocket.newBuilder("ws://websocket.example.com") .onReceiveText( (ws, text) 
-> System.out.println(text)) .onError( (ws, t) -> t.printStackTrace()) .onClose( (ws, msg) -> 
System.out.println("closing: " + msg))  .create();|
At some stage earlier I had a prototype that looked exactly what you've 
described
here.

One thing that caused me to discard it, was not lambda expressions themselves,
but the fact that we're splitting one type into several fine-grained ones. Some
simple examples showed me that non-trivial (useful) handlers would need to hold
and share some state. With split interfaces (an interface per function) we don't
have the kind of convenience and simplicity we would have with a single
interface.

Thus we have to organize this state/data sharing somewhere outside, making the
code a tiny bit more tangled and rigid (subjectively).

With functional interfaces and lambdas I feel it looks cleaner, but it might be
more painful to code against. I understand I might be overestimating the amount
of inconvenience here, so I would appreciate to hear your opinion on this.
A few points:
- I see it the other way, there are multiple functions being combined into a single type. - By making this a single class, the API forces the application to subclass and commits the concrete supertype.
   This is generally accepted as being undesirable.
- The application may very well want to have different code and structures for binary vs text data
   and to code them in different classes.


  The Builder methods can use generic java.util.function interfaces with the 
appropriate parameter types.
  Each of the callbacks should include the websocket,
Yes, I thought about it. Among other good things, it gives us most of the
required types for free (i.e. no need to create many tiny types).

The thing that I didn't like was that if we choose to go with anonymous classes,
we would be stuck with these overly generic method names: apply, accept, etc.
That might become awkward if we're going to use the same type but from different
perspectives.
The nice thing about functional interfaces (single method) is that they are matched on the structure of the signature, not on the names. For Lambdas, the names don't matter. With lambdas available, I doubt a developer would use anonymous classes anymore, though if the developer wants create their own explicit classes the generic method names apply.
(as you point out below)


For example, both read and write back pressure have essentially the same
interface and the same semantics. It's the same message. The difference is
whether one is receiving or sending. In this case -- which side of the
callback one is on. Either one is calling .suspend(boolean) or one is being
called as suspend(boolean).

(for illustrative purposes only):

/**
* A control used to communicate back-pressure to a peer.
*/
public interface Backpressure {

    /**
     * Tells peer to suspend/resume data flow.
     *
     * @param f {@code true} to suspend, {@code false} to resume
     */
    void suspend(boolean f);
}

Then WebSocket.Builder could have this:

/**
* Configures the builder with a user-provided back-pressure handler.
*
* <p> {@code WebSocket} will notify the user on whether they should
* suspend sending by calling {@code control.suspend(true)}, and {@code
* control.suspend(false)} when sending can be resumed.
*
* @param handler a back pressure handler
* @return this builder
*/
public Builder sendingBackpressure(Backpressure handler) {
    return this;
}

And WebSocket could have this:

/**
* Returns a back pressure control, a user can use to notify a server
* it should suspend/resume sending.
*
* @return a control to communicate an incoming back-pressure.
*/
public abstract Backpressure receivingBackpressure();

This type is already lambda-freindly (it's a functional interface). Now let's
change it to Consumer<Boolean>, just to not create an additional type.

Then in the builder you can easily hide Consumer's "accept" by providing a
method reference:

WebSocket ws = WebSocket.newBuilder("ws://websocket.example.com")
        .sendingBackpressure(this::onHold())

or lambda

WebSocket ws = WebSocket.newBuilder("ws://websocket.example.com")
        .sendingBackpressure(() -> onHold())

But not in the WebSocket:

        ws.receivingBackpressure().consume(true);

Generic functional interface is a good handler (callback), but a terrible domain
object.
I see your point but I don't think its an appropriate re-use of the Backpressure interface. The ws.receivingBackpressure method would have a boolean argument (t/f) to tell the WebSocket how to behave. The current WebSocket API is not structured to have a 'controls' style of API.

BTW, the suspendReceiving() and resumeReceiving() methods would be better as a single method with a boolean argument. If the application has some state itself about suspended state, it will need to use if (suspended) resumeRecieving() else suspendReceiving() to get the desired effect.


- The design to wrap the Outgoing data creates an extra step that makes the API
  a bit harder to use.  It would be more natural to add the sendAsync  methods 
to WebSocket
  and avoid the creation of extra objects.

|ws.sendAsync("hello"); ws.sendAsync(bytebuffer); // an alternate signature 
could use varargs, ByteBuffer data... |
I considered it some time ago. Here's the obvious disadvantage of not having
this step I've found: it mixes the way data is sent with the way data is
constructed. Previous prototypes had several different "send" methods:

        S = {send, sendAsync, queue,...}

We also have several different ways (4 at the moment) to construct data:

        D = {ByteBuffer, CharSequence, Iterator<CharSequence>, 
Iterator<ByteBuffer>, ...}

So if we put this into a single type we'll have to provide {S x D} methods:

send(ByteBuffer);
send(CharSequence);
send(Iterator<CharSequence>);
send(Iterator<ByteBuffer>);
...
queue(Iterator<ByteBuffer>);

Agreed, at the moment we have only 1 send and just 4 types of data sources. But
size S or D might change in future, and we'd better have a prepared way to adapt
to it.

I'm less worried about the number of methods, if the methods are named logically, the developer
will find the right method by completion in the IDE or know what they are.
I'm more concerned about extra steps the developer has to write code for that seem unnecessary compared to a straightforward implementation of the function.

Thanks, Roger

Reply via email to