Hi,
On Fri, Mar 25, 2016 at 5:21 PM, Pavel Rappo wrote:
> Hi,
>
> Could you please review my change for JDK-8087113
>
>http://cr.openjdk.java.net/~prappo/8087113/webrev.03/
>
> This webrev contains initial implementation and basic tests for WebSocket API.
> Specification conformance & interoperability testing has been performed with
> Autobahn Testsuite [1]. As for API tests, I will provide more of them later.
Throwing exceptions.
---
Given that the API is making use of CompletableFuture, throwing
exceptions instead is in my experience not the right thing to do.
You want to communicate failure via the CompletableFuture rather than
throwing exceptions.
For example, the WebSocket.sendXXX() methods throw a variety of
exceptions, mostly IllegalStateExceptions, for reasons that may not be
dependent on the application using the API (the connection is closed,
an error occurred, the permits are exhausted, etc.).
Throwing exceptions will make the API cumbersome to use because
applications have to do:
try {
CF cf = websocket.sendText(...);
} catch (Exception x) {
// What here ?
}
When using async API, you don't want to catch exceptions anymore,
since the (only) mechanism of conveying failure is the async API (via
CFs or callbacks, or whatever other means is there in the API).
IMHO the implementation should be rewritten to never throw exceptions,
and always notify failures via CFs.
Threading.
---
WebSocket.sendXXX() calls
MessagePublisher.send(), which dispatches a to
MessagePublisher.react(), which calls
MessageSender.onNext(), which dispatches to
MessageSender.react(), which calls
MessageSender.send(), which calls
Writer.onNext(), which dispatches to
Writer.write(), which finally writes the bytes to the network.
There are 3 (!) dispatches to different threads for a WebSocket send().
This is an enormous waste of resources which is completely unnecessary.
While I understand the goal of this implementation is not to be the
fastest ever written, dispatching every WebSocket send() to 3 threads
is not what I would expect from the JDK.
Send of WebSocket messages should be done in the caller thread, with
*zero* dispatches in between.
A read event from the network dispatches to
Reader.react(), which fake dispatches to
BuffersSubscriber.onNext() which dispatches to
BuffersSubscriber.react() which calls
Reader.readFrame() which calls
BuffersSubscriber.deliver() which fake dispatches to
WebSocket.Listener (application code).
There are 2 dispatches to different threads for every WebSocket read
event. I can live with 1 dispatch, 2 are unnecessary.
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.
I am also not sure that Reader is handling buffers correctly.
A buffer is used to read from the network, then "shared" (not sure
what is the semantic of this "sharing") and a slice passed to the
application.
Then the read loop comes over, and I understand that it may find the
read buffer consumed and dispose() it.
That disposed buffer can now be used to read more from the network,
overwriting the data previously read and therefore invalidating the
slice passed to the application.
I am also dubious about the usage of the CF returned by onXXX()
methods: if the read buffer is sliced and the slice passed to the
application, the underlying memory of both the read and sliced buffers
cannot be touched until the CF is completed.
I think that WebSocket.Listener javadoc need a clarification of the
exact semantic of the CF returned by onXXX() methods and provide a
couple of examples, like A) queueing messages without consuming them
(but still calling request(1)) and B) passing them to other
asynchronous APIs.
It also needs a clarification of what happens if an application never
returns (or returns after a long time) from onXXX() methods.
I kind of stopped here.
I understand this is a first draft, but IMHO the implementation needs
a major overhaul to simplify it.
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
