Re: RFR JDK-8157273: Simplify outgoing messages queueing policy in WebSocket API

[Chris Hegarty]

Looks fine.

-Chris.

On 09/06/16 11:54, Pavel Rappo wrote:

Hi,

Could you please review the following change for JDK-8157273?

http://cr.openjdk.java.net/~prappo/8157273/webrev.01/

This change addresses some WebSocket API refinements and enhancements from [1].

Currently WebSocket API allows only one scheduled but not yet sent message (i.e.
an outstanding send operation) at a time. This design was influenced by
java.nio.channels.AsynchronousChannel:

 *  Asynchronous channels are safe for use by multiple concurrent 
threads.
 * Some channel implementations may support concurrent reading and writing, 
but
 * may not allow more than one read and one write operation to be 
outstanding at
 * any given time.

Although it is both easy to understand and implement, this was found to not work
well with WebSocket API.

1. In WebSocket API it's not always easily known (in some modes of operation)
for an app when the previous send operation has completed. Thus maintaining a
non-overlapping sends is a burden on the user.

2. In contract with AsynchronousChannel, WebSocket does not allow "short writes"
(i.e. a situation when a Future returned by the send operation may complete
normally after the message has been written only partially.)
The CompletableFuture in WebSocket completes only after an error or when the
whole message has been written. Therefore, there's no inherent risk in
serializing sends in WebSocket.
The result on the wire will always be some permutation of messages, and not
their chunks.

Thanks,
-Pavel


[1] https://bugs.openjdk.java.net/browse/JDK-8155621

Re: RFR JDK-8157273: Simplify outgoing messages queueing policy in WebSocket API

[Simone Bordet]

Hi,

On Thu, Jun 9, 2016 at 3:20 PM, Pavel Rappo  wrote:
> Yes, you are correct. Here are some reasons behind this decision:
>
> 1. This is more friendly. One doesn't *have to* carefully build a tree of CS
> dependencies.

This is a bit weak.
On the other hand is less "friendly" because it allows application to
blow up the heap, and discover that only in production.

> 2. "One outstanding write" works perfect only in one-by-one mode:
>
>webSocket.sendX(message).thenRun(() -> webSocket.request(1))
>
> Though it might look good for a conference slide, as soon as we decide to use
> some other strategy, e.g. having a window akin to one used in this example in
> java.util.concurrent.Flow:
>
> *   public void onSubscribe(Subscription subscription) {
> * long initialRequestSize = bufferSize;
> * count = bufferSize - bufferSize / 2; // re-request when half consumed
> * (this.subscription = subscription).request(initialRequestSize);
> *   }
> *   public void onNext(T item) {
> * if (--count <= 0)
> *   subscription.request(count = bufferSize - bufferSize / 2);
> * consumer.accept(item);
> *   }
>
> it becomes a noticeably more difficult to maintain a strict non-overlapping
> order. A user will *have to* have their own queue of last `bufferSize` CFs.

This contradicts what you say down in bullet 4.
An application that calls request(n) where n > 1 should be prepared to
handle n items, either by buffering or by some other gathering
mechanism.
Such application may have a queue of CFs or a queue of items, but the
complexity is identical.

> 3. It's good for implementations. A performance obsessed implementation might
> decide to group several writes into one gathering write on the lowe level,
> squeezing extra microseconds out of latency and reducing the number of calls 
> to
> OS.

This may be true, but it will need to be measured.
It may well be that the additional queueing you have to do internally
(with possible additional allocation) obliterates the gains you have
with gathered writes.
And WebSocket being about low latency, you may not want to contend a
lock on a queue to add/gather items to write.

> 4. I don't think it somehow conflicts with the back-pressure we have in the 
> API.
> After all, it's up to a user how many outstanding writes they want to have. 
> The
> implementation controls *the incoming flow*, not allowing unrequested messages
> to be read/cached/stored. Well, it's a user's responsibility to keep an eye on
> their outgoing queue to not allow it to overflow. After all, the user is
> provided with everything to do this in a non-blocking async fashion!
>
> I would appreciate to hear from you on this.

With this bullet you are basically saying that it's applications'
responsibility to handle A) non-completed sequential writes and B)
concurrent writes.
If this holds, then an "at most one outstanding write" is as good as
any other policy, probably even better because the application will
not depend on some implementation decisions that may later change.

In summary, none of the reasons you listed above is valid for me to
justify this change.

Having said that, I think this is more an implementation problem than
an API problem.
The API supports both a model with an "at most one outstanding write"
as well as other models, such as:

* allow only 1 outstanding write from the same thread, but allow
concurrent writes
* allow a bounded number of outstanding writes from any thread
* allow an unbounded number of outstanding writes from any thread

FYI, we had this problem in javax.websocket, where Jetty chose one
policy, but Tomcat another.
Wrote a chat application using standard APIs against Jetty, worked
fine; deployed in Tomcat failed.
This resulted in changing the application to assume the minimum
behavior (i.e. at most 1 outstanding write).

If, for any reason, you have to change the implementation and go back
to an at most 1 outstanding write model, you will break all
applications out there.

Again FYI, API design is not only about the signature of the API, but
also about their semantic.
In Servlet 3.1 the semantic of
Servlet[Input|Output]Stream.[read|write]() was changed (from blocking
to non-blocking), despite no signature changes in the API.
This resulted in breaking all Servlet Filters implementation that were
wrapping the request or the response to process the content.

I think that having a stricter semantic such as at most 1 outstanding
write helps since applications will know exactly what to expect and
will be forced to think about handling non-completed sequential writes
and concurrent writes, and do that properly, and be in full control.

A wider semantic of unbounded outstanding writes may lead to write
applications that don't care about non-completed sequential writes and
concurrent writes, that may possibly blow up the heap in production.
The fix is the same as above, write the application thinking this
properly, but now there is a double work: the application que

Re: RFR JDK-8157273: Simplify outgoing messages queueing policy in WebSocket API

[Pavel Rappo]

> On 10 Jun 2016, at 11:09, Simone Bordet  wrote:
> 
> Hi,
> 
> On Thu, Jun 9, 2016 at 3:20 PM, Pavel Rappo  wrote:
>> Yes, you are correct. Here are some reasons behind this decision:
>> 
>> 1. This is more friendly. One doesn't *have to* carefully build a tree of CS
>> dependencies.
> 
> This is a bit weak.
> On the other hand is less "friendly" because it allows application to
> blow up the heap, and discover that only in production.

I think it wouldn't be wise to rely on API to ensure it will catch a potentially
bad behaviour for you. Even with a single outstanding write you cannot be sure
the implementation will not throw an IllegalStateException *sometime later* in
production due to a peculiar state you've missed while coding. How's that any
different from "blowing up the heap"? I hope no-one will suggest to catch ISE
and to "try again later".

The bottom line, it should not be a substitute of good battery of rigorous
tests.

If you want a guard dog, you can always have an atomic counter that keeps an eye
on the total number of outstanding sends. Increment it each time a sendX has
returned a CF. Decrement when the returned CF has completed. If at any time the
number goes above a certain threshold, throw an exception of your choice, log
the thing, send an important email, etc.

I suppose the vast majority of users will either use the one-by-one mode or will
initially request a Long.MAX_VALUE messages, thus reducing everything to a
simple callback push mode.

Queueing will work for both case, while "one-outstanding" write will only work
for the former one. 

>> 4. I don't think it somehow conflicts with the back-pressure we have in the 
>> API.
>> After all, it's up to a user how many outstanding writes they want to have. 
>> The
>> implementation controls *the incoming flow*, not allowing unrequested 
>> messages
>> to be read/cached/stored. Well, it's a user's responsibility to keep an eye 
>> on
>> their outgoing queue to not allow it to overflow. After all, the user is
>> provided with everything to do this in a non-blocking async fashion!
>> 
>> I would appreciate to hear from you on this.
> 
> With this bullet you are basically saying that it's applications'
> responsibility to handle A) non-completed sequential writes and B)
> concurrent writes.

Sorry Simone, could you please elaborate on what a "non-completed sequential
write" is? As for concurrent writes, the answer is no. Maybe this requires a bit
of spec clarification, we'll see. The point is Text and Binary messages are very
simple. They work in a simple FIFO. If the app ensures sendX are invoked in a
particular order:

   sendText(A), sendText(B), sendText(C), ...

then the messages will be sent in the same order:

   A, B, C...

Ping, Pong and Close are as you know a bit different. These are so called
"control" messages. They can be interjected in between frames of a "non-control"
message (e.g. Text or Binary).

I'm sure the API should be open for this behaviour. And in this case the only
way to enforce a particular order is to correlate sends with a completions of
previous ones. Not returns of CFs, but completions of CFs. For example, if I
want to make sure a Close message is sent *after* the huge Text, then I do this:

webSocket.sendText("In the beginning God created the heaven and the 
earth...")
 .thenCompose(WebSocket::sendClose);

rather than this:

webSocket.sendText("In the beginning God created the heaven and the 
earth...");
webSocket.sendClose();

With one outstanding write we close the door to the possibility of interjecting.

> Having said that, I think this is more an implementation problem than
> an API problem.
> The API supports both a model with an "at most one outstanding write"
> as well as other models, such as:
> 
> * allow only 1 outstanding write from the same thread, but allow
> concurrent writes
> * allow a bounded number of outstanding writes from any thread
> * allow an unbounded number of outstanding writes from any thread
> 
> FYI, we had this problem in javax.websocket, where Jetty chose one
> policy, but Tomcat another.
> Wrote a chat application using standard APIs against Jetty, worked
> fine; deployed in Tomcat failed.
> This resulted in changing the application to assume the minimum
> behavior (i.e. at most 1 outstanding write).
> 
> If, for any reason, you have to change the implementation and go back
> to an at most 1 outstanding write model, you will break all
> applications out there.
> 
> Again FYI, API design is not only about the signature of the API, but
> also about their semantic.

Exactly. I've never said otherwise. API is an interconnected system of types
with well-defined semantics. Ideally implementations should be arbitrarily
substitutable. Akin to "is-a" relationship with their LSP.

> In Servlet 3.1 the semantic of
> Servlet[Input|Output]Stream.[read|write]() was changed (from blocking
> to non-blocking), despite no signature changes in the API.
> This resulte

RFR 8071660 :URLPermission not handling empty method lists correctly

[vyom]

Hi All,

Please review the below fix.

Bug   : JDK-8071660 URLPermission not handling empty method 
lists correctly
Webrev : 
http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html 



Thanks,
Vyom

RFR 8114860: Behavior of java.net.URLPermission.getActions() contradicts spec

[vyom]

Hi All,

Please review the below fix.

Bug   : JDK-8114860 Behavior of 
java.net.URLPermission.getActions() contradicts spec
Webrev : 
http://cr.openjdk.java.net/~vtewari/8114860/webrev0.0/index.html 



Thanks,
Vyom