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

[Vyom Tewari]

Hi Pavel,

Please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8114860/webrev0.2/index.html 
). I 
reverted the commented "hashtest" test.


Thanks,
Vyom


On Monday 20 June 2016 10:53 PM, Pavel Rappo wrote:

Same here. I got this only after I had a look at the history of changes.


On 20 Jun 2016, at 17:53, Vyom Tewari  wrote:

I was not expecting that hashcodetest is testing NPE by comparing the 
"hardcoded" hash values.

Re: RFR 8144008: Setting NO_PROXY on an URLConnection is not complied with

[Chris Hegarty]

Yom,

Looking at this again, with fresh eyes, before pushing it for you. I think the
change would be clearer if it explicitly used Proxy.NO_PROXY, rather than
relying on the fact that the `proxy` field is initialized to NO_PROXY. Since
the intent here is to ALWAYS make an non-proxied connection.

@@ -192,11 +192,11 @@

  * The following method, createSocket, is provided to allow the
  * https client to override it so that it may use its socket factory
  * to create the socket.
  */
 protected Socket createSocket() throws IOException {

-return new java.net.Socket();
+return new java.net.Socket(Proxy.NO_PROXY));

 }
 
 protected InetAddress getLocalAddress() throws IOException {
 if (serverSocket == null)
 throw new IOException("not connected”);

-Chris.

> On 16 Jun 2016, at 21:09, Chris Hegarty  wrote:
> 
> 
>> On 16 Jun 2016, at 18:55, Roger Riggs  wrote:
>> 
>> Hi Vyom,
>> 
>> Looks ok,
> 
> +1.
> 
> I attempted to add a test for https, and just realised that the 
> javax.net.SocketFactory
> does not support Socket(Proxy).  It may be worth capturing this issue 
> somewhere, but
> it is way beyond the scope of this bug, and less of an issue with the new 
> HTTP 
> Client alternative.
> 
> -Chris.
> 
>> Roger
>> 
>> On 6/16/2016 10:35 AM, Vyom Tewari wrote:
>>> Hi All,
>>> 
>>> Please find the latest 
>>> webrev(http://cr.openjdk.java.net/~vtewari/8144008/webrev0.1/index.html 
>>> ), i 
>>> got some off line comments from Chris.
>>> 
>>> Thanks,
>>> Vyom
>>> 
>>> On Tuesday 14 June 2016 12:11 PM, Vyom Tewari wrote:
 Hi All,
 
 Please review the below fix.
 Bug   : JDK-8144008 Setting NO_PROXY on an URLConnection is not 
 complied with
 Webrev : 
 http://cr.openjdk.java.net/~vtewari/8144008/webrev0.0/index.html 
 
 
 Thanks,
 Vyom
 
>>> 
>> 
> 

Re: Preliminary RFR JDK-8159053: Improve onPing/onClose behavior

[Chris Hegarty]

I think this is good.

Just some ideas to simplify the wording.

*  After a Close message has been received, the implementation will
* close the connection automatically. However, the client may finish
* sending the current sequence of partially sent message parts, if any.
* To facilitate this, the WebSocket implementation will only
* attempt to close the connection at the earliest of, either the
* completion of the returned {@code CompletionStage} or the last part
* of the current message has been sent.

Otherwise, the simplifications are welcome.

-Chris.

> On 17 Jun 2016, at 16:15, Pavel Rappo  wrote:
> 
> Hi,
> 
> Could you please [*] review the following change for the upcoming JDK-8159053?
> This change addresses:
> 
> 1. Listener.onClose/onPing behaviour, making the implementation fully*
> responsible of protocol compliance. So reactions on onClose/onPing cannot be
> overridden/redefined in a Listener
> 
> 2. Simpler representation of the Close message.
> 
 ….
> 
>/**
> * Receives a Close message.
> *
> * …..
> *
> *  After a Close message has been received, the implementation will
> * close the connection automatically. However, the client may finish
> * sending the current sequence of partially sent message parts, if any.
> * To facilitate this, the WebSocket implementation will only
> * attempt to close the connection at the earliest of, either the
> * completion of the returned {@code CompletionStage} or the last part
> * of the current message has been sent.
> *
> * @implSpec The default implementation behaves as if:
> *
> * {@code
> * return CompletableFuture.completedStage(null);
> * }
> *
> * @param webSocket
> * the WebSocket
> * @param statusCode
> * the status code
> * @param reason
> * the reason
> *
> * @return a CompletionStage that when completed will trigger closing of
> * the WebSocket
> */
>default CompletionStage onClose(WebSocket webSocket,
>   int statusCode,
>   String reason) {
>return null;
>}
> 
>/**
> * Sends a Close message with the given status code and the reason.
> *
> *  Returns a {@code CompletableFuture} which completes
> * normally when the message has been sent or completes exceptionally if an
> * error occurs.
> *
> *  The {@code statusCode} is an integer in the range {@code 1000 <= 
> code
> * <= 4999}, except for {@code 1004}, {@code 1005}, {@code 1006} and {@code
> * 1015}. The {@code reason} is a short string that must have an UTF-8
> * representation not longer than {@code 123} bytes.
> *
> *  For more information about status codes see
> * https://tools.ietf.org/html/rfc6455#section-7.4";>RFC 6455, 
> 7.4. Status Codes.)
> *
> *  If a Close message has been already sent or the {@code WebSocket} is
> * closed, then invoking this method has no effect and returned {@code
> * CompletableFuture} completes normally.
> *
> *  The returned {@code CompletableFuture} can complete exceptionally
> * with:
> * 
> *  {@link IOException}
> *  if an I/O error occurs during this operation
> * 
> *
> * @param statusCode
> * the status code
> * @param reason
> * the reason
> *
> * @return a CompletableFuture with this WebSocket
> *
> * @throws IllegalArgumentException
> * if the {@code statusCode} has an illegal value, or
> * {@code reason} doesn't have an UTF-8 representation not longer
> * than {@code 123} bytes
> *
> * @see #sendClose()
> */
>CompletableFuture sendClose(int statusCode, String reason);
> 
>/**
> * Sends an empty Close message.
> *
> *  Returns a {@code CompletableFuture} which completes
> * normally when the message has been sent or completes exceptionally if an
> * error occurs.
> *
> *  If a Close message has been already sent or the {@code WebSocket} is
> * closed, then invoking this method has no effect and returned {@code
> * CompletableFuture} completes normally.
> *
> *  The returned {@code CompletableFuture} can complete exceptionally
> * with:
> * 
> *  {@link IOException}
> *  if an I/O error occurs during this operation
> * 
> *
> * @return a CompletableFuture with this WebSocket
> */
>CompletableFuture sendClose();
> 
> Thanks,
> -Pavel
> 
> 
> [1] Please excuse me for not publishing this in a form of a webrev. The reason
> is that currently there are too many outstanding changes in the queue that 
> would
> simply obscure the review.

Re: RFR 8144008: Setting NO_PROXY on an URLConnection is not complied with

[Vyom Tewari]

Hi Chris,

I am fine with suggested change( return new 
java.net.Socket(Proxy.NO_PROXY)); )as this will improve the code 
readability.


Thanks,
Vyom

On Tuesday 21 June 2016 01:38 PM, Chris Hegarty wrote:

Yom,

Looking at this again, with fresh eyes, before pushing it for you. I think the
change would be clearer if it explicitly used Proxy.NO_PROXY, rather than
relying on the fact that the `proxy` field is initialized to NO_PROXY. Since
the intent here is to ALWAYS make an non-proxied connection.

@@ -192,11 +192,11 @@

   * The following method, createSocket, is provided to allow the
   * https client to override it so that it may use its socket factory
   * to create the socket.
   */
  protected Socket createSocket() throws IOException {

-return new java.net.Socket();
+return new java.net.Socket(Proxy.NO_PROXY));

  }
  
  protected InetAddress getLocalAddress() throws IOException {

  if (serverSocket == null)
  throw new IOException("not connected”);

-Chris.


On 16 Jun 2016, at 21:09, Chris Hegarty  wrote:



On 16 Jun 2016, at 18:55, Roger Riggs  wrote:

Hi Vyom,

Looks ok,

+1.

I attempted to add a test for https, and just realised that the 
javax.net.SocketFactory
does not support Socket(Proxy).  It may be worth capturing this issue 
somewhere, but
it is way beyond the scope of this bug, and less of an issue with the new HTTP
Client alternative.

-Chris.


Roger

On 6/16/2016 10:35 AM, Vyom Tewari wrote:

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8144008/webrev0.1/index.html 
), i got 
some off line comments from Chris.

Thanks,
Vyom

On Tuesday 14 June 2016 12:11 PM, Vyom Tewari wrote:

Hi All,

Please review the below fix.
Bug   : JDK-8144008 Setting NO_PROXY on an URLConnection is not 
complied with
Webrev : http://cr.openjdk.java.net/~vtewari/8144008/webrev0.0/index.html 


Thanks,
Vyom

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

[Pavel Rappo]

Looks good to me. Thanks.

> On 21 Jun 2016, at 08:10, Vyom Tewari  wrote:
> 
> Hi Pavel,
> 
> Please find the updated 
> webrev(http://cr.openjdk.java.net/~vtewari/8114860/webrev0.2/index.html 
> ). I 
> reverted the commented "hashtest" test.
> 
> Thanks,
> Vyom
> 
> 
> On Monday 20 June 2016 10:53 PM, Pavel Rappo wrote:
>> Same here. I got this only after I had a look at the history of changes.
>> 
>>> On 20 Jun 2016, at 17:53, Vyom Tewari  wrote:
>>> 
>>> I was not expecting that hashcodetest is testing NPE by comparing the 
>>> "hardcoded" hash values.
> 

Re: Preliminary RFR JDK-8159053: Improve onPing/onClose behavior

[Pavel Rappo]

Thanks Chris, I will update the wording and prepare the actual RFR.

> On 21 Jun 2016, at 09:43, Chris Hegarty  wrote:
> 
> I think this is good.
> 
> Just some ideas to simplify the wording.
> 
>*  After a Close message has been received, the implementation will
>* close the connection automatically. However, the client may finish
>* sending the current sequence of partially sent message parts, if any.
>* To facilitate this, the WebSocket implementation will only
>* attempt to close the connection at the earliest of, either the
>* completion of the returned {@code CompletionStage} or the last part
>* of the current message has been sent.
> 
> Otherwise, the simplifications are welcome.
> 
> -Chris.
> 
>> On 17 Jun 2016, at 16:15, Pavel Rappo  wrote:
>> 
>> Hi,
>> 
>> Could you please [*] review the following change for the upcoming 
>> JDK-8159053?
>> This change addresses:
>> 
>> 1. Listener.onClose/onPing behaviour, making the implementation fully*
>> responsible of protocol compliance. So reactions on onClose/onPing cannot be
>> overridden/redefined in a Listener
>> 
>> 2. Simpler representation of the Close message.
>> 
> ….
>> 
>>   /**
>>* Receives a Close message.
>>*
>>* …..
>>*
>>*  After a Close message has been received, the implementation will
>>* close the connection automatically. However, the client may finish
>>* sending the current sequence of partially sent message parts, if any.
>>* To facilitate this, the WebSocket implementation will only
>>* attempt to close the connection at the earliest of, either the
>>* completion of the returned {@code CompletionStage} or the last part
>>* of the current message has been sent.
>>*
>>* @implSpec The default implementation behaves as if:
>>*
>>* {@code
>>* return CompletableFuture.completedStage(null);
>>* }
>>*
>>* @param webSocket
>>* the WebSocket
>>* @param statusCode
>>* the status code
>>* @param reason
>>* the reason
>>*
>>* @return a CompletionStage that when completed will trigger closing of
>>* the WebSocket
>>*/
>>   default CompletionStage onClose(WebSocket webSocket,
>>  int statusCode,
>>  String reason) {
>>   return null;
>>   }
>> 
>>   /**
>>* Sends a Close message with the given status code and the reason.
>>*
>>*  Returns a {@code CompletableFuture} which completes
>>* normally when the message has been sent or completes exceptionally if an
>>* error occurs.
>>*
>>*  The {@code statusCode} is an integer in the range {@code 1000 <= 
>> code
>>* <= 4999}, except for {@code 1004}, {@code 1005}, {@code 1006} and {@code
>>* 1015}. The {@code reason} is a short string that must have an UTF-8
>>* representation not longer than {@code 123} bytes.
>>*
>>*  For more information about status codes see
>>* https://tools.ietf.org/html/rfc6455#section-7.4";>RFC 6455, 
>> 7.4. Status Codes.)
>>*
>>*  If a Close message has been already sent or the {@code WebSocket} is
>>* closed, then invoking this method has no effect and returned {@code
>>* CompletableFuture} completes normally.
>>*
>>*  The returned {@code CompletableFuture} can complete exceptionally
>>* with:
>>* 
>>*  {@link IOException}
>>*  if an I/O error occurs during this operation
>>* 
>>*
>>* @param statusCode
>>* the status code
>>* @param reason
>>* the reason
>>*
>>* @return a CompletableFuture with this WebSocket
>>*
>>* @throws IllegalArgumentException
>>* if the {@code statusCode} has an illegal value, or
>>* {@code reason} doesn't have an UTF-8 representation not longer
>>* than {@code 123} bytes
>>*
>>* @see #sendClose()
>>*/
>>   CompletableFuture sendClose(int statusCode, String reason);
>> 
>>   /**
>>* Sends an empty Close message.
>>*
>>*  Returns a {@code CompletableFuture} which completes
>>* normally when the message has been sent or completes exceptionally if an
>>* error occurs.
>>*
>>*  If a Close message has been already sent or the {@code WebSocket} is
>>* closed, then invoking this method has no effect and returned {@code
>>* CompletableFuture} completes normally.
>>*
>>*  The returned {@code CompletableFuture} can complete exceptionally
>>* with:
>>* 
>>*  {@link IOException}
>>*  if an I/O error occurs during this operation
>>* 
>>*
>>* @return a CompletableFuture with this WebSocket
>>*/
>>   CompletableFuture sendClose();
>> 
>> Thanks,
>> -Pavel
>> 
>> 
>> [1] Please excuse me for not publishing this in a form of a webrev. The 
>> reason
>> is that curr

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi Pavel,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.4/index.html 
). I 
had incorporated the review comments.


Thanks,
Vyom


On Monday 20 June 2016 07:27 PM, Pavel Rappo wrote:

Vyom, please consider the following changes:

1. Append 8071660 to the lists of tests here:

* @bug 8010464 8027570 8027687 8029354 8071660
^
2. Reformat the code the way it's in the rest of the file:

from:

  266 URLPermission that = (URLPermission)p;
  267
  268 if(this.methods.isEmpty() && !that.methods.isEmpty())
  269 return false;
  270
  271 if (!this.methods.isEmpty() && !this.methods.get(0).equals("*") &&
  272 Collections.indexOfSubList(this.methods, that.methods) == 
-1) {
  273 return false;
  274 }

to:

  266 URLPermission that = (URLPermission)p;
  267
  268 if (this.methods.isEmpty() && !that.methods.isEmpty()) {
  269 return false;
  270 }
  271
  272 if (!this.methods.isEmpty() &&
  273 !this.methods.get(0).equals("*") &&
  274 Collections.indexOfSubList(this.methods,
  275that.methods) == -1) {
  276 return false;
  277 }

Other than these minor things, looks good.

Thanks,
-Pavel


On 20 Jun 2016, at 12:36, Daniel Fuchs  wrote:

On 17/06/16 15:09, Vyom Tewari wrote:

Hi,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html
). I
fixed the typo along with spaces issue.

Looks good!

-- daniel


Thanks,
Vyom


On Friday 17 June 2016 06:34 PM, Roger Riggs wrote:

Hi Vyom,

URLPermissionTest.java:

line 125:  it looks odd to assign the same variable twice with the
same value.
   Perhaps it should have been assigning this.url2 = url2.

line 330-332: please add a space after "," in argument lists.

Roger




On 6/17/2016 8:42 AM, Daniel Fuchs wrote:

On 17/06/16 13:25, Vyom Tewari wrote:

Hi Daniel,

thanks for review please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html
) .
I added some more tests where base url is different.

Thanks Vyom,
Looks good to me.

See if you can get someone more experienced in the area
to give a thumb up too :-)

best regards,

-- daniel



Thanks,
Vyom

On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:

On 17/06/16 09:21, Vyom Tewari wrote:

Hi All,

Please find the new
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html

).

I  addressed the review comments given by Daniel.

Hi Vyom,

Looks good to me - but I'm a bit concerned that the previous
mistake was not caught that by any test.
Could you add a test that fails with you previous fix
but passes with the new one?

best regards,

-- daniel


Thanks,
Vyom


On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:

Hi Vyom,

This looks strange to me:

268 if(!this.methods.isEmpty() && that.methods.isEmpty())
269 return true;
270 if(this.methods.isEmpty() && !that.methods.isEmpty())
271 return false;
272 if(this.methods.isEmpty() && that.methods.isEmpty())
273 return true;

Namely, lines 269 & 273 will return true before the URL part
of the permission has been checked.
Is that really the expected behavior?

best regards,

-- daniel

On 11/06/16 05:50, vyom wrote:

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

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Chris Hegarty]

The code changes look fine, but the bigger question is whether we 
want to support actions without any ‘method’ being specified,  like
“:X-Bar”? Should the constructor throw IAE, or is that even possible
now, it would require a spec clarification ( would that invalidate existing
code already doing this ) ?


-Chris.

> On 21 Jun 2016, at 11:16, Vyom Tewari  wrote:
> 
> Hi Pavel,
> 
> Please find the latest 
> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.4/index.html 
> ). I had 
> incorporated the review comments.
> 
> Thanks,
> Vyom
> 
> 
> On Monday 20 June 2016 07:27 PM, Pavel Rappo wrote:
>> Vyom, please consider the following changes:
>> 
>> 1. Append 8071660 to the lists of tests here:
>> 
>>  * @bug 8010464 8027570 8027687 8029354 8071660
>>^
>> 2. Reformat the code the way it's in the rest of the file:
>> 
>> from:
>> 
>>  266 URLPermission that = (URLPermission)p;
>>  267
>>  268 if(this.methods.isEmpty() && !that.methods.isEmpty())
>>  269 return false;
>>  270
>>  271 if (!this.methods.isEmpty() && !this.methods.get(0).equals("*") 
>> &&
>>  272 Collections.indexOfSubList(this.methods, that.methods) 
>> == -1) {
>>  273 return false;
>>  274 }
>> 
>> to:
>> 
>>  266 URLPermission that = (URLPermission)p;
>>  267
>>  268 if (this.methods.isEmpty() && !that.methods.isEmpty()) {
>>  269 return false;
>>  270 }
>>  271
>>  272 if (!this.methods.isEmpty() &&
>>  273 !this.methods.get(0).equals("*") &&
>>  274 Collections.indexOfSubList(this.methods,
>>  275that.methods) == -1) {
>>  276 return false;
>>  277 }
>> 
>> Other than these minor things, looks good.
>> 
>> Thanks,
>> -Pavel
>> 
>>> On 20 Jun 2016, at 12:36, Daniel Fuchs  wrote:
>>> 
>>> On 17/06/16 15:09, Vyom Tewari wrote:
 Hi,
 
 Please find the latest
 webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html
 ). I
 fixed the typo along with spaces issue.
>>> Looks good!
>>> 
>>> -- daniel
>>> 
 Thanks,
 Vyom
 
 
 On Friday 17 June 2016 06:34 PM, Roger Riggs wrote:
> Hi Vyom,
> 
> URLPermissionTest.java:
> 
> line 125:  it looks odd to assign the same variable twice with the
> same value.
>   Perhaps it should have been assigning this.url2 = url2.
> 
> line 330-332: please add a space after "," in argument lists.
> 
> Roger
> 
> 
> 
> 
> On 6/17/2016 8:42 AM, Daniel Fuchs wrote:
>> On 17/06/16 13:25, Vyom Tewari wrote:
>>> Hi Daniel,
>>> 
>>> thanks for review please find the latest
>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html
>>> ) .
>>> I added some more tests where base url is different.
>> Thanks Vyom,
>> Looks good to me.
>> 
>> See if you can get someone more experienced in the area
>> to give a thumb up too :-)
>> 
>> best regards,
>> 
>> -- daniel
>> 
>> 
>>> Thanks,
>>> Vyom
>>> 
>>> On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:
 On 17/06/16 09:21, Vyom Tewari wrote:
> Hi All,
> 
> Please find the new
> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html
> 
> ).
> 
> I  addressed the review comments given by Daniel.
 Hi Vyom,
 
 Looks good to me - but I'm a bit concerned that the previous
 mistake was not caught that by any test.
 Could you add a test that fails with you previous fix
 but passes with the new one?
 
 best regards,
 
 -- daniel
 
> Thanks,
> Vyom
> 
> 
> On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:
>> Hi Vyom,
>> 
>> This looks strange to me:
>> 
>> 268 if(!this.methods.isEmpty() && that.methods.isEmpty())
>> 269 return true;
>> 270 if(this.methods.isEmpty() && !that.methods.isEmpty())
>> 271 return false;
>> 272 if(this.methods.isEmpty() && that.methods.isEmpty())
>> 273 return true;
>> 
>> Namely, lines 269 & 273 will return true before the URL part
>> of the permission has been checked.
>> Is that really the expected behavior?
>> 
>> best regards,
>> 
>> -- daniel
>> 
>> On 11/06/16 05:50, vyom wrote:

Re: RFR JDK-8156742: Miscellaneous WebSocket API improvements

[Pavel Rappo]

Hi, 

Let me try again to propose the following set of changes:

http://cr.openjdk.java.net/~prappo/8156742/webrev.02/

The difference between this version and the previous one is that there are no
controversial changes to onClose method [*]. This version also removes
`sendText(Stream message)` which is not really essential
and indeed can be added later (if needed.)

In other words, this version address what's been discussed in the thread after
the previous version has been sent.

Thanks for your help,
-Pavel


[*] Please note that onClose/sendClose/onPing changes will be addressed in the
upcoming https://bugs.openjdk.java.net/browse/JDK-8159053

> On 31 May 2016, at 18:25, Pavel Rappo  wrote:
> 
> Hi,
> 
> Could you please review the following change for JDK-8156742?
> 
> http://cr.openjdk.java.net/~prappo/8156742/webrev.01/
> 
> This change addresses the first group of WebSocket API refinements and
> enhancements from [1].
> 
> 1. Change method `Builder#connectTimeout(long, TimeUnit)` to 
> `Builder#connectTimeout(Duration)`
> 
> Make use of convenience introduced with java.time API. The builder is not a
> performance-critical place, so there's no harm in constructing an object of
> `java.time.Duration` each time it's needed. Moreover, since 9, there's a 
> bridge
> between TimeUnit and Duration: java.util.concurrent.TimeUnit.toChronoUnit
> 
> 2. Change method `long WebSocket#request(long)` to `void 
> WebSocket#request(long)`
> 
> Otherwise a detail of implementation becomes a part of the spec. In this case
> it's not desirable, since we'll have to specify the behaviour in the corner 
> case
> (long wrap) and force future implementations to maintain this abstraction.
> 
> 3. Remove method `WebSocket#sendBinary(byte[], boolean)`
> 
> This method provides not enough convenience to justify its existence.
> 
> 4. Change type `CloseCode` for `CloseReason` that aggregates both status code
> and close reason.
> 
> Current `Listener.onClose` looks ugly. It hides the otherwise explicit to all
> WebSocket users knowledge that 'reason' string can't go without the
> 'status code', i.e.:
> 
> (statusCode reason?)?
> 
> CloseReason types fuses both entities into a single type. As a bonus all
> knowledge about status code and reason string formats is now bound to a single
> place.
> 
> 5. Specify `WebSocket#sendClose` idempotency
> 
> Not producing IllegalStateException upon an attempt to close an already closed
> WebSocket seems to be a user-friendly solution. It's already an established 
> practice
> in the JDK, e.g. java.nio.channels.SocketChannel.shutdownOutput
> 
> 6. A number of miscellaneous editorial changes, missing copyright headers, 
> tests.
> 
> Thanks,
> -Pavel
> 
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8155621
> 

Re: RFR JDK-8156742: Miscellaneous WebSocket API improvements

[Chris Hegarty]

> On 21 Jun 2016, at 12:21, Pavel Rappo  wrote:
> 
> Hi, 
> 
> Let me try again to propose the following set of changes:
> 
> http://cr.openjdk.java.net/~prappo/8156742/webrev.02/

This mainly looks fine, just a small comment:

 - WebSocket.java
   "Or to close abruptly call {@link #abort()}.” Rather than start a sentence
with ‘Or’, maybe “Alternatively, the {@link #abort() abort} method can be
invoked/used to close abruptly”.


> The difference between this version and the previous one is that there are no
> controversial changes to onClose method [*]. This version also removes
> `sendText(Stream message)` which is not really 
> essential
> and indeed can be added later (if needed.)

Yes, if it becomes clear that this is indeed useful, then I agree
it can be added later.

> In other words, this version address what's been discussed in the thread after
> the previous version has been sent.
> 
> Thanks for your help,
> -Pavel
> 
> 
> [*] Please note that onClose/sendClose/onPing changes will be addressed in the
>upcoming https://bugs.openjdk.java.net/browse/JDK-8159053

Sounds fine.

-Chris.

>> On 31 May 2016, at 18:25, Pavel Rappo  wrote:
>> 
>> Hi,
>> 
>> Could you please review the following change for JDK-8156742?
>> 
>> http://cr.openjdk.java.net/~prappo/8156742/webrev.01/
>> 
>> This change addresses the first group of WebSocket API refinements and
>> enhancements from [1].
>> 
>> 1. Change method `Builder#connectTimeout(long, TimeUnit)` to 
>> `Builder#connectTimeout(Duration)`
>> 
>> Make use of convenience introduced with java.time API. The builder is not a
>> performance-critical place, so there's no harm in constructing an object of
>> `java.time.Duration` each time it's needed. Moreover, since 9, there's a 
>> bridge
>> between TimeUnit and Duration: java.util.concurrent.TimeUnit.toChronoUnit
>> 
>> 2. Change method `long WebSocket#request(long)` to `void 
>> WebSocket#request(long)`
>> 
>> Otherwise a detail of implementation becomes a part of the spec. In this case
>> it's not desirable, since we'll have to specify the behaviour in the corner 
>> case
>> (long wrap) and force future implementations to maintain this abstraction.
>> 
>> 3. Remove method `WebSocket#sendBinary(byte[], boolean)`
>> 
>> This method provides not enough convenience to justify its existence.
>> 
>> 4. Change type `CloseCode` for `CloseReason` that aggregates both status code
>> and close reason.
>> 
>> Current `Listener.onClose` looks ugly. It hides the otherwise explicit to all
>> WebSocket users knowledge that 'reason' string can't go without the
>> 'status code', i.e.:
>> 
>> (statusCode reason?)?
>> 
>> CloseReason types fuses both entities into a single type. As a bonus all
>> knowledge about status code and reason string formats is now bound to a 
>> single
>> place.
>> 
>> 5. Specify `WebSocket#sendClose` idempotency
>> 
>> Not producing IllegalStateException upon an attempt to close an already 
>> closed
>> WebSocket seems to be a user-friendly solution. It's already an established 
>> practice
>> in the JDK, e.g. java.nio.channels.SocketChannel.shutdownOutput
>> 
>> 6. A number of miscellaneous editorial changes, missing copyright headers, 
>> tests.
>> 
>> Thanks,
>> -Pavel
>> 
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8155621
>> 
> 

Re: RFR JDK-8156742: Miscellaneous WebSocket API improvements

[Roger Riggs]

+1 with Chris' suggested rewording.

Roger



On 6/21/2016 7:45 AM, Chris Hegarty wrote:

On 21 Jun 2016, at 12:21, Pavel Rappo  wrote:

Hi,

Let me try again to propose the following set of changes:

http://cr.openjdk.java.net/~prappo/8156742/webrev.02/

This mainly looks fine, just a small comment:

  - WebSocket.java
"Or to close abruptly call {@link #abort()}.” Rather than start a sentence
 with ‘Or’, maybe “Alternatively, the {@link #abort() abort} method can be
 invoked/used to close abruptly”.



The difference between this version and the previous one is that there are no
controversial changes to onClose method [*]. This version also removes
`sendText(Stream message)` which is not really essential
and indeed can be added later (if needed.)

Yes, if it becomes clear that this is indeed useful, then I agree
it can be added later.


In other words, this version address what's been discussed in the thread after
the previous version has been sent.

Thanks for your help,
-Pavel


[*] Please note that onClose/sendClose/onPing changes will be addressed in the
upcoming https://bugs.openjdk.java.net/browse/JDK-8159053

Sounds fine.

-Chris.


On 31 May 2016, at 18:25, Pavel Rappo  wrote:

Hi,

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

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

This change addresses the first group of WebSocket API refinements and
enhancements from [1].

1. Change method `Builder#connectTimeout(long, TimeUnit)` to
`Builder#connectTimeout(Duration)`

Make use of convenience introduced with java.time API. The builder is not a
performance-critical place, so there's no harm in constructing an object of
`java.time.Duration` each time it's needed. Moreover, since 9, there's a bridge
between TimeUnit and Duration: java.util.concurrent.TimeUnit.toChronoUnit

2. Change method `long WebSocket#request(long)` to `void 
WebSocket#request(long)`

Otherwise a detail of implementation becomes a part of the spec. In this case
it's not desirable, since we'll have to specify the behaviour in the corner case
(long wrap) and force future implementations to maintain this abstraction.

3. Remove method `WebSocket#sendBinary(byte[], boolean)`

This method provides not enough convenience to justify its existence.

4. Change type `CloseCode` for `CloseReason` that aggregates both status code
and close reason.

Current `Listener.onClose` looks ugly. It hides the otherwise explicit to all
WebSocket users knowledge that 'reason' string can't go without the
'status code', i.e.:

(statusCode reason?)?

CloseReason types fuses both entities into a single type. As a bonus all
knowledge about status code and reason string formats is now bound to a single
place.

5. Specify `WebSocket#sendClose` idempotency

Not producing IllegalStateException upon an attempt to close an already closed
WebSocket seems to be a user-friendly solution. It's already an established 
practice
in the JDK, e.g. java.nio.channels.SocketChannel.shutdownOutput

6. A number of miscellaneous editorial changes, missing copyright headers, 
tests.

Thanks,
-Pavel


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

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Chris Hegarty]

On 21 Jun 2016, at 11:21, Chris Hegarty  wrote:
> 
> The code changes look fine, but the bigger question is whether we 
> want to support actions without any ‘method’ being specified,  like
> “:X-Bar”? Should the constructor throw IAE, or is that even possible
> now, it would require a spec clarification ( would that invalidate existing
> code already doing this ) ?

Seems like a corner case and not worth trying to contort the spec to
cover it. Such a permission is effectively useless anyway.

I’ll sponsor this change for you.

-Chris.

> 
> -Chris.
> 
>> On 21 Jun 2016, at 11:16, Vyom Tewari  wrote:
>> 
>> Hi Pavel,
>> 
>> Please find the latest 
>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.4/index.html 
>> ). I had 
>> incorporated the review comments.
>> 
>> Thanks,
>> Vyom
>> 
>> 
>> On Monday 20 June 2016 07:27 PM, Pavel Rappo wrote:
>>> Vyom, please consider the following changes:
>>> 
>>> 1. Append 8071660 to the lists of tests here:
>>> 
>>> * @bug 8010464 8027570 8027687 8029354 8071660
>>>   ^
>>> 2. Reformat the code the way it's in the rest of the file:
>>> 
>>> from:
>>> 
>>> 266 URLPermission that = (URLPermission)p;
>>> 267
>>> 268 if(this.methods.isEmpty() && !that.methods.isEmpty())
>>> 269 return false;
>>> 270
>>> 271 if (!this.methods.isEmpty() && !this.methods.get(0).equals("*") 
>>> &&
>>> 272 Collections.indexOfSubList(this.methods, that.methods) 
>>> == -1) {
>>> 273 return false;
>>> 274 }
>>> 
>>> to:
>>> 
>>> 266 URLPermission that = (URLPermission)p;
>>> 267
>>> 268 if (this.methods.isEmpty() && !that.methods.isEmpty()) {
>>> 269 return false;
>>> 270 }
>>> 271
>>> 272 if (!this.methods.isEmpty() &&
>>> 273 !this.methods.get(0).equals("*") &&
>>> 274 Collections.indexOfSubList(this.methods,
>>> 275that.methods) == -1) {
>>> 276 return false;
>>> 277 }
>>> 
>>> Other than these minor things, looks good.
>>> 
>>> Thanks,
>>> -Pavel
>>> 
 On 20 Jun 2016, at 12:36, Daniel Fuchs  wrote:
 
 On 17/06/16 15:09, Vyom Tewari wrote:
> Hi,
> 
> Please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html
> ). I
> fixed the typo along with spaces issue.
 Looks good!
 
 -- daniel
 
> Thanks,
> Vyom
> 
> 
> On Friday 17 June 2016 06:34 PM, Roger Riggs wrote:
>> Hi Vyom,
>> 
>> URLPermissionTest.java:
>> 
>> line 125:  it looks odd to assign the same variable twice with the
>> same value.
>>  Perhaps it should have been assigning this.url2 = url2.
>> 
>> line 330-332: please add a space after "," in argument lists.
>> 
>> Roger
>> 
>> 
>> 
>> 
>> On 6/17/2016 8:42 AM, Daniel Fuchs wrote:
>>> On 17/06/16 13:25, Vyom Tewari wrote:
 Hi Daniel,
 
 thanks for review please find the latest
 webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html
 ) .
 I added some more tests where base url is different.
>>> Thanks Vyom,
>>> Looks good to me.
>>> 
>>> See if you can get someone more experienced in the area
>>> to give a thumb up too :-)
>>> 
>>> best regards,
>>> 
>>> -- daniel
>>> 
>>> 
 Thanks,
 Vyom
 
 On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:
> On 17/06/16 09:21, Vyom Tewari wrote:
>> Hi All,
>> 
>> Please find the new
>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html
>> 
>> ).
>> 
>> I  addressed the review comments given by Daniel.
> Hi Vyom,
> 
> Looks good to me - but I'm a bit concerned that the previous
> mistake was not caught that by any test.
> Could you add a test that fails with you previous fix
> but passes with the new one?
> 
> best regards,
> 
> -- daniel
> 
>> Thanks,
>> Vyom
>> 
>> 
>> On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:
>>> Hi Vyom,
>>> 
>>> This looks strange to me:
>>> 
>>> 268 if(!this.methods.isEmpty() && that.methods.isEmpty())
>>> 269 return true;
>>> 270 if(this.methods.isEmpty() && !that.methods.isEmpty())
>>> 271 return false;
>>> 272 if(this.methods.isEmpty() && that.