Re: RFR [9] 8085785: sun/net/www/protocol/http/ZoneId.java timeout intermittently
Looks fine to me also Chris. Regards, Sean. On 30/05/2016 22:52, Mark Sheppard wrote: Hi Chris, this looks good ... so the server now listens on wildcard and the client uses IPv6 loopback as the destination address. The use of NO_PROXY, is good. I wouldn't have thought of that, and in the past, Tests have experienced firewall issues on linux and macos perviously without this setting. regards Mark On 26/05/2016 14:33, Chris Hegarty wrote: ZoneId is attempting to verify the 'Host' header set by the HttpURLConnection implementation when given an IPv6 literal containing a scope id. It does so by iterating the network interfaces on the machine attempting to find one that is suitable to use as the listening interface for a temporary test HTTP sever. Then it connects to it, and verifies the value of the 'Host' header. This is problematic as some interfaces like teredo on Windows, or awdl on Mac, are not suitable. Rather than all this, the test can use the IPv6 loopback address, which can contain a scope id, if retrieved from the NetworkInterface API. http://cr.openjdk.java.net/~chegar/8085785/ https://bugs.openjdk.java.net/browse/JDK-8085785 -Chris.
RFR JDK-8156742: Miscellaneous WebSocket API improvements
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
Pavel, On Tue, May 31, 2016 at 7:25 PM, Pavel Rappo wrote: > Hi, > > Could you please review the following change for JDK-8156742? > > http://cr.openjdk.java.net/~prappo/8156742/webrev.01/ Comments on the API only. Looks much cleaner, good job. I like WebSocket.request(long) being reinstated, and the Listener methods taking a WebSocket as first parameter, Builder.header() being sane :) Further comments: * What is interface Text for ? If it does perform a bytes-to-chars conversion, then offering asByteBuffer() can be easily done by the application. A websocket implementation must check that the incoming text bytes are indeed UTF-8. Doing the check is equivalent to creating the corresponding String, so I'm not sure Text is of any help. * CloseReason I would rename it to CloseInfo, as CloseReason hints to me of the reason only, not the code. I think this class exposes too many failure codes that applications *must not* be able to use. For example, 1002 is not something that the application can ever send, only the implementation can, and having a public API to create a 1002 CloseInfo is not something you I'd like to see exposed. Same goes with 1007, which the implementation must detect, not the application; etc. I would probably just leave CloseInfo.of(), with the current checks you are doing extended. * onClose() semantic. I am not sure why CloseInfo is wrapped with an Optional ? Can't the implementation just synthesize a (1005, "") instead and get rid of the Optional ? Also, I think it should return a CF, for the following reason. Notification of onClose() is a half-close. Applications are entitled to send data from within onClose(). For such reason, the implementation cannot send the response close frame just after the method returns. It should wait until the application has finished writing, and hence the need for the CF. * sendText(Stream message); I think it's too much for a utility method. It's a rare use case, I don't think it's worth it. Applications that wrap the default WebSocket object will have to implement it. ws.sendText(stream.collect(joining())) is equivalent and as short. If the goal was to send one frame per string, there is almost zero chance that the exact fragmentation is maintained at the server, so once again I don't see the reason for this method. * Extensions I don't recall if extensions have been ruled out ? Browsers seem to have settled at implementing permessage-deflate. 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
Ping - RFR 8158023: SocketExceptions contain too little information sometimes
Hi, ping - any comments on this? Thanks Christoph From: Langer, Christoph Sent: Freitag, 27. Mai 2016 10:30 To: net-dev@openjdk.java.net Cc: core-libs-...@openjdk.java.net; nio-...@openjdk.java.net Subject: RFR 8158023: SocketExceptions contain too little information sometimes Hi all, please review the following change: Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8158023.1/ Bug: https://bugs.openjdk.java.net/browse/JDK-8158023 During error analysis I stumbled over a place where I encountered a SocketException which was thrown along with some strerror information as message. I found it hard to find the originating code spot with that information. So I looked at the places where we throw exceptions, namely JNU_Throw... and NET_Throw... functions and came up with the following enhancement: - NET_ThrowByNameWithLastError can go completely as it does not provide any benefit over JNU_ThrowByNameWithLastError. - JNU_ThrowByNameWithLastError can be cleaned up. - I added JNU_ThrowByNameWithMessageAndLastError to print out a string like message + ": " + last error. - I went over all places where NET_ThrowByNameWithLastError is used and replaced it appropriately. Do you think this change is desirable/possible? Though it's mainly a net topic, I'm posting it to nio-dev and core-libs-dev as well as JNU_Throw... code affects all. Best regards Christoph
