Re: RFR JDK-8087113: Websocket API and implementation
Hi Pavel, several comments: 1. WebSocket.request(long n) is documented as " @throws IllegalArgumentException if n < -1 " It looks meaningless to allow 0. 2. Some concern on the way of handling close. Consider following scenario. * obtain a ws connection * message communications balabala... * initialize closing handshake on either client/server peer * if the client side didn't request adequate size or not requested at all, it actually doesn't know connection is closed. "onClose" will never be invoked, because close message is at the end of "queue". * it looks to me that: o the closing handshake can be blocked by UN-requested messages. o a websocket can be in a state that isClosed() return false but it can't send any message. Note that the state is not transient. Filed a bug https://bugs.openjdk.java.net/browse/JDK-8156191. It looks current design treats CloseMessage in similar way with other messages. I'm not sure if it is worth something like WSClosingHandshake. 3. I'm concerning the bug in #2 is side-effect of reactive mode. May I ask what is the benefit of reactive mode. WebSocket.request(long n) seem to be good in first glance but: * It looks end users need to calculate how many messages are expected. The another question comes o what messages will be taken into account. How about PING/PONG or else? * We may use request( a big value up to Long.MAX). But, in this case, why we need it? This looks simple but a bit problematic. My $0.02, Felix On 2016/5/3 23:23, Pavel Rappo wrote: Hello, Here's an updated webrev with the latest implementation: http://cr.openjdk.java.net/~prappo/8087113/webrev.04/ If you're going to review it, please be advised that there is a number of known API and implementation issues yet to be resolved [1]. More API tests will be provided in the next couple of months. All applicable tests (1.1.1-10.1.1, 305 in total) from Autobahn Testsuite [2] are green. Thanks, -Pavel [1] https://bugs.openjdk.java.net/browse/JDK-8155621 [2] http://autobahn.ws/testsuite/ On 25 Mar 2016, at 16:21, 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. Thanks, -Pavel [1] http://autobahn.ws/testsuite/
Re: RFR JDK-8087113: Websocket API and implementation
> On 6 May 2016, at 09:16, Felix Yang wrote:
>
> Hi Pavel,
> several comments:
>
> 1. WebSocket.request(long n) is documented as "
>
> @throws IllegalArgumentException if n < -1
> "
>
> It looks meaningless to allow 0.
First of all, the way `request` is defined now is by no means meaningless.
* @return resulting unfulfilled demand with this request taken into account
*/
long request(long n);
Passing zero allows you, for instance, to get current demand without modifying
it.
But, this is going to be removed [1] (WebSocket API tasks, item 10).
Think of the ability to pass 0 as of a degree of robustness. That's a corner
case. It allows you to put some computed value, not necessarily a literal as it
does not require you to check the value for 0. As far as it is non-negative, no
harm is done.
If you're still not convinced, consider any of the following:
"abc".substring(1, 1);
collection.removeAll(Collections.emptyList());
inputStream.read(new byte[0]);
byteChannel.read(ByteBuffer.allocate(0));
> 2. Some concern on the way of handling close. Consider following scenario.
>
> • obtain a ws connection
> • message communications balabala...
> • initialize closing handshake on either client/server peer
> • if the client side didn't request adequate size or not requested at
> all, it actually doesn't know connection is closed. "onClose" will never be
> invoked, because close message is at the end of "queue".
True. That's the cost of providing reactive back-pressure. Otherwise we would
have to read off the socket in the background looking for the Close message to
appear.
> • it looks to me that:
> • the closing handshake can be blocked by UN-requested
> messages.
> • a websocket can be in a state that isClosed() return false
> but it can't send any message. Note that the state is not transient.
WebSocket is not necessarily closed after you send a Close message:
/**
* Tells whether the {@code WebSocket} is closed.
*
* A {@code WebSocket} deemed closed when either the underlying socket
* is closed or the closing handshake is completed.
*
* @return {@code true} if the {@code WebSocket} is closed,
* {@code false} otherwise
*/
boolean isClosed();
> Filed a bug https://bugs.openjdk.java.net/browse/JDK-8156191. It looks
> current design treats CloseMessage in similar way with other messages. I'm
> not sure if it is worth something like WSClosingHandshake.
> 3. I'm concerning the bug in #2 is side-effect of reactive mode. May I ask
> what is the benefit of reactive mode. WebSocket.request(long n) seem to be
> good in first glance but:
> • It looks end users need to calculate how many messages are expected.
> The another question comes
> • what messages will be taken into account. How about PING/PONG
> or else?
> • We may use request( a big value up to Long.MAX). But, in this case,
> why we need it? This looks simple but a bit problematic.
You can read on benefits of reactive mode here [2, 3]. And feel free to read the
API thread [4] as well.
[1] https://bugs.openjdk.java.net/browse/JDK-8155621
[2] http://www.reactive-streams.org
[3] https://github.com/reactive-streams/reactive-streams-jvm/
[4] http://mail.openjdk.java.net/pipermail/net-dev/2015-August/009077.html
http://mail.openjdk.java.net/pipermail/net-dev/2015-September/009079.html
http://mail.openjdk.java.net/pipermail/net-dev/2015-October/009223.html
Re: RFR: 8155888: java/net/httpclient/QuickResponses.java intermittently failed with java.util.ConcurrentModificationException
On 5 May 2016, at 12:29, Michael McMahon wrote: > Another occasional test case failure. It's a concurrent modification > exception caused > by modifying a list during processing of the list (by the same thread). The > solution > is to keep separate lists of the modifications and to process them after the > iterator > completes. > > http://cr.openjdk.java.net/~michaelm/8155888/webrev.1/ This looks ok to me Michael. -Chris.
Re: RFR: 8155888: java/net/httpclient/QuickResponses.java intermittently failed with java.util.ConcurrentModificationException
+1 On 5/6/2016 1:04 PM, Chris Hegarty wrote: On 5 May 2016, at 12:29, Michael McMahon wrote: Another occasional test case failure. It's a concurrent modification exception caused by modifying a list during processing of the list (by the same thread). The solution is to keep separate lists of the modifications and to process them after the iterator completes. http://cr.openjdk.java.net/~michaelm/8155888/webrev.1/ This looks ok to me Michael. -Chris.
