I'm still working my way through the code, but ...
On 04/04/16 15:02, Pavel Rappo wrote:
Hi Simone, thanks for deep dive into this!
On 3 Apr 2016, at 18:43, Simone Bordet <simone.bor...@gmail.com> wrote:
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.
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.
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.
I agree. There is certainly a good case for "some" exceptions being
thrown by the caller thread. For me, I think Pavel got the balance
right here.
BTW, whether or not a single outstanding write is appropriate here is a
completely different question.
It is.
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
POSSIBLY
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.
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.
Dispatch:
1 - Some simply invariant checks, then allow the calling thread
to return.
2 - If text decode. Mask payload.
3 - Write the actual bytes to the socket
With larger messages, then being able to separate 2 & 3 above seems
desirable. Seems ok to me.
Send of WebSocket messages should be done in the caller thread, with
*zero* dispatches in between.
Since this is an async API, I don't see how 1) above can, or should,
be avoided.
What's the reason behind this? What about being asynchronous, non-blocking and
responsive? Also please remember since we use ExecutorService, a task does not
necessarily correspond to a separate thread. Actually all this could run with a
same thread executor (try it!):
@Override
public void execute(Runnable command) {
command.run();
}
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)?
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.
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.
(Same arguments as the above)
+1 especially with TEXT
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.
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.
Shared<Buffer> represents a Buffer whose contents are sliced without overlaps.
Given the slices are then published safely to their target threads they can be
simultaneously read and/or write.
The root buffer (the one that hosts all the slices) counts its children. Each
dispose() decrements total count and does nothing, unless the resulting count
is 0. I which case this Shared<Buffer> (the parent) goes back to the pool.
Simple
reference counting, no magic. In theory should provide better latency and more
concurrency without any need for copying.
Maybe it (dispose) just needs a better name?
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.
True for the sliced buffer. Not sure I understand why it should be the case for
the read buffer.
I've just sliced a part that represents something deliverable, perfect, why
can't I proceed with reading the next chunk of data?
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.
+1
-Chris.
