RFR JDK-8087113: Websocket API and implementation

[Simone Bordet]

Hi,

On Mon, Apr 4, 2016 at 4:02 PM, Pavel Rappo <pavel.rappo at oracle.com> wrote:
> I see your point. Yes, working asynchronously kind of suggests that we should
> relay all exceptions through the asynchronous interface (CF in this particular
> case), simply because otherwise we would have to handle exceptions in many
> places (like you have illustrated).
>
> But there's a force working in the opposite direction. The further the thrown
> exception is from it's origin, the more of its context we loose.

I don't know what you mean here.

> In addition to this, some exceptions would clearly mean clear programmatic
> errors, that could easily be detected on the spot. E.g. passing null, or a
> negative value, where the positive or 0 is expected. Or a state problem. E.g.
> trying to send more data after a Close message has been already sent. The same
> with permits exhausting (let's now forget you're looking into the implementation
> rather than in the API). In the API only 1 incomplete/outstanding/queued write
> is permitted at any given time. It's a an application's fault if it doesn't keep
> an eye on whether or not the previous send has been completed. I can hardly
> imagine what could be done in the catch block above in this case.

In my experience, it does not matter.
We went the way of throwing exceptions in case of API misuse by the
application, only to be bitten multiple times: "oh darn, an exception
has been thrown, that callback/CF was nested, did not get notified,
the whole system halted because of that missing notification via
callback/CF".
We reimplemented and never did it again, and lived happier since.

You have designed an API that returns CFs, but then you also say "oh,
btw we don't notify the CF but we can throw exceptions".
That's a mixed design that is not friendly with users, no matter what
you think is an "application" error.
Application writers are now forced to wrap *every* single API call
with a try/catch because they don't know if you are going to notify
the CF or throw an exception.
Try to combine 2 CFs where one is from the WebSocket APIs.
Yuck!

> All these are the stages of the "pipeline", each of which is independent to a
> degree. I've tried to break unnecessary temporal coupling between them.

It's an questionable implementation choice, and not necessary.
You said what you have done, not why you did it, what pros and cons
you weighed, etc.

>> Send of WebSocket messages should be done in the caller thread, with
>> *zero* dispatches in between.
>
> What's the reason behind this?

Imagine if to write to a stream, you have to do:

new Thread(() -> new Thread(() -> new Thread(() ->
stream.write()).start()).start()).start();

What would you think if you saw such code ?
That's akin to what the current implementation does.

To be clear, the reason behind it is to not waste an enormous amount
of resources, and simplify the implementation.
Imagine you're writing 20k messages/s and you have to dispatch every
send() 3 times.

> What about being asynchronous, non-blocking and
> responsive?

I don't understand ?
By dispatching 3 times you don't achieve neither of those qualities,
quite the contrary in fact.

> I had my reasons behind this while implementing. Would you agree it's generally
> more desirable first to make something work correctly and then to make it run
> fastER (the former is not mutually exclusive to the latter)?

There are no reasons to put into the JDK an implementation that wastes
so many resources, especially when it's simpler to write it with
*zero* dispatches.

Your comment about "make it run then make it fast" is totally out of
context here.
You don't write an implementation that is 10 times more complex than
it needs to be, just to make this kind of comments.
And you have shown zero proof that your implementation is correct.

> By introducing the SignalHandler I separated an invocation from the actual
> execution, thus making all components behave more like an Active Object.
> Initially I was kind of scared about running into hardly trackable deadlocks
> and/or unnecessarily thread blocks.

Imagine that !
How about removing all dispatches and poof ! you have a *mathematical*
proof that there are no deadlocks because there is only 1 thread
involved.
Right now, you cannot prove that you have removed all deadlocks.

>> I am not sure that Reader is implementing Flow (ReactiveStreams) correctly.
>> Reader.react() calls subscriber.onNext() from thread T1.
>> The subscriber handles the message and calls request(1), which calls
>> handler.signal() which dispatches T2 to Reader.react() so now there
>> are 2 threads inside Reader.react(), which is bad.
>
> First of all, java.net.http.Reader#react has the following structure:
>
>     private void react() {
>         synchronized (this) {
>         ...
>         }
>     }
>
> Therefore it's not possible for more than a single thread to be inside
> Reader.react() at any given time. Secondly, if you have a look at SignalHandler
> more closely you will see that it doesn't allow a handler action (supplied in
> the constructor) to be executed simultaneously by many threads. The
> synchronization here therefore is purely for memory visibility purposes.

Right, I forgot that you're doing I/O operations while holding a lock.

> I've just sliced a part that represents something deliverable, perfect, why
> can't I proceed with reading the next chunk of data?

When the read buffer has no more space, you are going to require a
read interest for no reason.
If you are otherwise compacting the buffer, you will run into the data
corruption problem I mentioned.

Bottom line is that the I/O read and write are naturally single
threaded and require little or no synchronization, nor dispatches, nor
exception throwing.
Sorry, but I am really confused about why you chose to write such a
complicated and resource wasteful implementation.

Thanks !

-- 
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz