Re: 8217461: (ch) Add Net.available to return the number of bytes in the socket input buffer
* Alan Bateman: > This is a small change to add a method to sun.nio.ch.Net to get the > number of bytes in the socket input buffer. The motive for adding this > to make it possible for the socket adaptors to implement > InputStream::available and also to support an alternative SocketImpl > based on NIO. Do you plan to read from the socket buffer in the implementation of available()? The problem is that even if there is currently data in the socket buffer, further data that arrives later can clear it. I think this can only happen as part of a connection reset. Maybe that's okay because reading will not block, but throw an exception instead? Thanks, Florian
Re: 8217461: (ch) Add Net.available to return the number of bytes in the socket input buffer
On 23/01/2019 09:42, Florian Weimer wrote: : Do you plan to read from the socket buffer in the implementation of available()? The problem is that even if there is currently data in the socket buffer, further data that arrives later can clear it. I think this can only happen as part of a connection reset. Maybe that's okay because reading will not block, but throw an exception instead? Not planning for available to read. The long standing behavior in this area is for SIS.available to consistently return 0 after a connection reset has been detected. This is of course awkward to replicate in a new implementation as it requires detecting connection reset, something that was never implemented in the SocketChannel implementation (mostly because it's highly platform/implementation specific as to how read behaves when attempt after a connection reset). -Alan
Re: 8217461: (ch) Add Net.available to return the number of bytes in the socket input buffer
* Alan Bateman: > On 23/01/2019 09:42, Florian Weimer wrote: >> : >> Do you plan to read from the socket buffer in the implementation of >> available()? The problem is that even if there is currently data in the >> socket buffer, further data that arrives later can clear it. I think >> this can only happen as part of a connection reset. Maybe that's okay >> because reading will not block, but throw an exception instead? >> > Not planning for available to read. The long standing behavior in this > area is for SIS.available to consistently return 0 after a connection > reset has been detected. This is of course awkward to replicate in a > new implementation as it requires detecting connection reset, > something that was never implemented in the SocketChannel > implementation (mostly because it's highly platform/implementation > specific as to how read behaves when attempt after a connection > reset). Sorry, what I meant is that available() says that there are bytes, and then when you try to read them, you get an exception because they are no longer there. I doubt that's the intent behind the InputStream::available specification, but as I said, it still avoids blocking, so it may match a literal interpretation. But presumably this is already the behavior with SocketInputStream? Thanks, Florian
RFR: 8217264: HttpClient: Blocking operations in mapper function do not work as documented
Hi, Please find below a fix for: 8217264: HttpClient: Blocking operations in mapper function do not work as documented [1] https://bugs.openjdk.java.net/browse/JDK-8217264 webrev: [2] http://cr.openjdk.java.net/~dfuchs/webrev_8217264/webrev.00/ The issue here is that if you try to map an InputStream to a GZIPInputStream in a mapper function provided to BodySubscribers::mapping, the response will be wedged. This is because the constructor of GZIPInputStream calls InputStream::read which blocks the current thread. The fix is to ensure that a new executor task will be used when calling BodySubscriber::getBody - which is the point at which the mapping function gets executed, in order to avoid blocking the current task. This is not a completely satisfactory solution, as doing blocking operations in a mapper function will take a thread out of the Executor's pool until the blocking operation eventually succeeds - which might be until the last byte of the response is received (or worse?), and might end up starving the HttpClient of available threads. I believe the documentation of BodySubscribers::mapping should instead promote safer behaviour - and steer users out of executing blocking operations in the mapper function. I have logged [3] JDK-8217627 to that effect. Even if we fix the API documentation as in [3] - and discourage blocking operation in the mapper, I still believe we should support the occasional blocking mapper functions (with the understanding that the response might get wedged if client's executor doesn't have sufficient free threads) - if only to allow mapping from InputStream to GZIPInputStream when the Executor is a sufficiently sized thread pool executor. This is the effect of my proposed fix [2]. best regards, -- daniel [1] https://bugs.openjdk.java.net/browse/JDK-8217264 [2] http://cr.openjdk.java.net/~dfuchs/webrev_8217264/webrev.00/ [3] https://bugs.openjdk.java.net/browse/JDK-8217627
RFR: 8216986 Remove unused code from SocksSocketImpl
Hi Could I get the following webrev reviewed please? It is just to remove dead code from SocksSocketImpl. Most of the code was an (unused) attempt to implement SOCKS for ServerSockets. getLocalPort() was potentially buggy and should not override the super class implementation. So, that was removed too. A couple of other orphaned methods are removed also. http://cr.openjdk.java.net/~michaelm/8216986/webrev.1/ Thanks, Michael.
Re: 8217461: (ch) Add Net.available to return the number of bytes in the socket input buffer
On 23/01/2019 10:55, Florian Weimer wrote: : Sorry, what I meant is that available() says that there are bytes, and then when you try to read them, you get an exception because they are no longer there. I doubt that's the intent behind the InputStream::available specification, but as I said, it still avoids blocking, so it may match a literal interpretation. But presumably this is already the behavior with SocketInputStream? It's very platform specific as to whether you can read bytes in the socket buffer when a connection reset is reported. This was something we discussed on net-dev last year in the context of other changes to detach the write path from interference due to connection reset. Classic networking has always attempted to mask the differences by remembering the connection reset so that subsequent calls to read work consistently across platforms. There's nothing in the proposed changes that touches or changes the long standing behavior. As regards reading after available returns a position value then the spec can only says that it doesn't block. It can't guarantee that the read doesn't throw I/O exception. Calling close between available and read is another scenario where read will fail. -Alan
Re: java.net.http.HttpClient: invalid exception when bad status line is returned
Hello, is this ML the correct place to report problems with HttpClient? If not, can you please point me to the right place? Thanks. > On 18 Jan 2019, at 23:57, Dmitry Sivachenko wrote: > > Hello, > > I am tasting java.net.http.HttpClient with > > openjdk version "11.0.1" 2018-10-16 > OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.1+13) > OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.1+13, mixed mode) > > on Mac OS X 10.14.2. > > Consider the case when server responds with bad (invalid format) status line: > "HTTP/1.1 FOO" > > The following IllegalArgumentException is thrown: > > Exception in thread "main" java.lang.IllegalArgumentException: For input > string: "FOO" > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:551) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > at ru.mitya.test.App.main(App.java:14) > Caused by: java.lang.NumberFormatException: For input string: "FOO" > at > java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:65) > at java.base/java.lang.Integer.parseInt(Integer.java:652) > at java.base/java.lang.Integer.parseInt(Integer.java:770) > at > java.net.http/jdk.internal.net.http.Http1HeaderParser.readStatusLineFeed(Http1HeaderParser.java:197) > at > java.net.http/jdk.internal.net.http.Http1HeaderParser.parse(Http1HeaderParser.java:124) > at > java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.handle(Http1Response.java:672) > at > java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.handle(Http1Response.java:603) > at > java.net.http/jdk.internal.net.http.Http1Response$Receiver.accept(Http1Response.java:594) > at > java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.tryAsyncReceive(Http1Response.java:650) > at > java.net.http/jdk.internal.net.http.Http1AsyncReceiver.flush(Http1AsyncReceiver.java:228) > at > java.net.http/jdk.internal.net.http.common.SequentialScheduler$SynchronizedRestartableTask.run(SequentialScheduler.java:175) > at > java.net.http/jdk.internal.net.http.common.SequentialScheduler$CompleteRestartableTask.run(SequentialScheduler.java:147) > at > java.net.http/jdk.internal.net.http.common.SequentialScheduler$SchedulableTask.run(SequentialScheduler.java:198) > at > java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) > at > java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) > at java.base/java.lang.Thread.run(Thread.java:834) > > > instead of the expected ProtocolException: > > Exception in thread "main" java.io.IOException: Invalid status line: > "HTTP/1.1 FOO" > at > java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) > at > java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) > at ru.mitya.test.App.main(App.java:14) > Caused by: java.net.ProtocolException: Invalid status line: "HTTP/1.1 FOO" > <...> > > The suggested fix would be to catch IllegalArgumentException thrown by > Integer.parseInt() in readStatusLineFeed() and throw > ProtocolException instead of it. > > What do you think? > > Thanks.
Re: java.net.http.HttpClient: invalid exception when bad status line is returned
Hi Dmitry, Thanks for reporting this. Right - the IllegalArgumentException is a bit strange there, and an IOException (or subclass of it) wrapping the NumberFormatException would be more appropriate. If you have OpenJDK author status and a JBS account then you could log a bug at https://bugs.openjdk.java.net/ otherwise then https://bugs.java.com/bugdatabase/ would be the place. best regards, -- daniel On 23/01/2019 12:16, Dmitry Sivachenko wrote: Hello, is this ML the correct place to report problems with HttpClient? If not, can you please point me to the right place? Thanks. On 18 Jan 2019, at 23:57, Dmitry Sivachenko wrote: Hello, I am tasting java.net.http.HttpClient with openjdk version "11.0.1" 2018-10-16 OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.1+13) OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.1+13, mixed mode) on Mac OS X 10.14.2. Consider the case when server responds with bad (invalid format) status line: "HTTP/1.1 FOO" The following IllegalArgumentException is thrown: Exception in thread "main" java.lang.IllegalArgumentException: For input string: "FOO" at java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:551) at java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) at ru.mitya.test.App.main(App.java:14) Caused by: java.lang.NumberFormatException: For input string: "FOO" at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:65) at java.base/java.lang.Integer.parseInt(Integer.java:652) at java.base/java.lang.Integer.parseInt(Integer.java:770) at java.net.http/jdk.internal.net.http.Http1HeaderParser.readStatusLineFeed(Http1HeaderParser.java:197) at java.net.http/jdk.internal.net.http.Http1HeaderParser.parse(Http1HeaderParser.java:124) at java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.handle(Http1Response.java:672) at java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.handle(Http1Response.java:603) at java.net.http/jdk.internal.net.http.Http1Response$Receiver.accept(Http1Response.java:594) at java.net.http/jdk.internal.net.http.Http1Response$HeadersReader.tryAsyncReceive(Http1Response.java:650) at java.net.http/jdk.internal.net.http.Http1AsyncReceiver.flush(Http1AsyncReceiver.java:228) at java.net.http/jdk.internal.net.http.common.SequentialScheduler$SynchronizedRestartableTask.run(SequentialScheduler.java:175) at java.net.http/jdk.internal.net.http.common.SequentialScheduler$CompleteRestartableTask.run(SequentialScheduler.java:147) at java.net.http/jdk.internal.net.http.common.SequentialScheduler$SchedulableTask.run(SequentialScheduler.java:198) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:834) instead of the expected ProtocolException: Exception in thread "main" java.io.IOException: Invalid status line: "HTTP/1.1 FOO" at java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) at java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) at ru.mitya.test.App.main(App.java:14) Caused by: java.net.ProtocolException: Invalid status line: "HTTP/1.1 FOO" <...> The suggested fix would be to catch IllegalArgumentException thrown by Integer.parseInt() in readStatusLineFeed() and throw ProtocolException instead of it. What do you think? Thanks.
Re: RFR: 8216986 Remove unused code from SocksSocketImpl
On 23/01/2019 11:44, Michael McMahon wrote: Hi Could I get the following webrev reviewed please? It is just to remove dead code from SocksSocketImpl. Most of the code was an (unused) attempt to implement SOCKS for ServerSockets. getLocalPort() was potentially buggy and should not override the super class implementation. So, that was removed too. A couple of other orphaned methods are removed also. http://cr.openjdk.java.net/~michaelm/8216986/webrev.1/ This is a good cleanup. Seems wrong for getLocalPort to return the "remote" address of the SOCKS server so that fix looks correct. Does it need a test? At some point I think we need to get this SocketImpl changed so that it delegates rather than extends PlainSocketImpl. I suspect that work will identify a few other oddities with SOCKS connections that may not be obvious now. -Alan
Re: RFR: 8216986 Remove unused code from SocksSocketImpl
On 23/01/2019, 12:37, Alan Bateman wrote: On 23/01/2019 11:44, Michael McMahon wrote: Hi Could I get the following webrev reviewed please? It is just to remove dead code from SocksSocketImpl. Most of the code was an (unused) attempt to implement SOCKS for ServerSockets. getLocalPort() was potentially buggy and should not override the super class implementation. So, that was removed too. A couple of other orphaned methods are removed also. http://cr.openjdk.java.net/~michaelm/8216986/webrev.1/ This is a good cleanup. Seems wrong for getLocalPort to return the "remote" address of the SOCKS server so that fix looks correct. Does it need a test? I can add a test for that. At some point I think we need to get this SocketImpl changed so that it delegates rather than extends PlainSocketImpl. I suspect that work will identify a few other oddities with SOCKS connections that may not be obvious now. Yes, that's what I plan to do next. Thanks, Michael.
RFR 8217657: Move the test for default value of jdk.includeInExceptions into own test
Hi, please review a small test update. Bug: https://bugs.openjdk.java.net/browse/JDK-8217657 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8217657.0/ I'd like to move the test for the correct default value of security property "jdk.includeInExceptions" into an own testcase in the jdk.security area. This seems a bit more natural than to hide this check in a java/net specific test and will help finding/maintaining tests when the (default-)value for that property changes. For instance new values get added or other OpenJDK builds have different defaults (e.g. SAPMachine). Thanks Christoph
Re: java.net.http.HttpClient: invalid exception when bad status line is returned
> On 23 Jan 2019, at 15:32, Daniel Fuchs wrote: > > Hi Dmitry, > > Thanks for reporting this. > > Right - the IllegalArgumentException is a bit strange there, > and an IOException (or subclass of it) wrapping the NumberFormatException > would be more appropriate. > > If you have OpenJDK author status and a JBS account then you > could log a bug at https://bugs.openjdk.java.net/ > otherwise then https://bugs.java.com/bugdatabase/ > would be the place. > Thanks! I followed second link and filed bug report #9059077 We have more bugs for HttpClient to report, this was the simplest one, I will report them one at a time.
RE: RFR 8217657: Move the test for default value of jdk.includeInExceptions into own test
Hi Christoph, it is a good idea to keep testing these two matters separately. Could you please document in the new test that in OpenJDK it is decided to keep this property empty? Something like: @comment In OpenJDK, this property is empty by default and on purpose. This test assures the default is not changed. Otherwise, looks good. Best regards, Goetz. From: net-dev On Behalf Of Langer, Christoph Sent: Mittwoch, 23. Januar 2019 17:06 To: OpenJDK Dev list ; OpenJDK Network Dev list Subject: [CAUTION] RFR 8217657: Move the test for default value of jdk.includeInExceptions into own test Hi, please review a small test update. Bug: https://bugs.openjdk.java.net/browse/JDK-8217657 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8217657.0/ I'd like to move the test for the correct default value of security property "jdk.includeInExceptions" into an own testcase in the jdk.security area. This seems a bit more natural than to hide this check in a java/net specific test and will help finding/maintaining tests when the (default-)value for that property changes. For instance new values get added or other OpenJDK builds have different defaults (e.g. SAPMachine). Thanks Christoph
