NIO based SocketImpl to replace legacy PlainSocketImpl
I've created a branch in the sandbox, named "niosocketimpl-branch", with a prototype SocketImpl implementation based on the infrastructure in sun.nio.ch package that supports the NIO channel implementations. The branch also includes the changes to java.net.Socket and ServerSocket to use this SocketImpl by default. There are several motivations for this prototype: The existing PlainSocketImpl is not fit for purpose in the potential future world of fibers that park instead of blocking carrier threads in syscalls. As I'm sure most people are on net-dev are aware, PlainSocketImpl is horrible to maintain due to the complexity of the code paths in its native methods. Replacing PlainSocketImpl, along with its associates SocketInputStream and SocketOutputStream, bring other benefits such as not needing to use the thread stack for I/O and not needing the fd table data structure to support asynchronous close. We have to be a cautious about replacing the PlainSocket/SIS/SOS after 20 years of service. It is likely that there is behavior in unspecified areas and corner case scenarios that existing applications or libraries may depend upon. There is performance work to do but it's possible there are cases where the performance might degrade, e.g. many years ago there were attempts to fix a synchronization issue in PlainSocketImpl that lead to complaints from usages involving hundreds of threads blocking on ServerSocket::accept. To that end, we are planning to evolve the prototype to allow the old and new SocketImpls to co-exist, maybe switched by a system property or some other means. Going there may require a bit yak shaving, for example in the SOCKS and HTTP proxy implementations as they need to be re-worked to forward rather than sub-class. If the prototype goes further then the idea is that both implementations could be included in the JDK for a few releases before removing the old code. The prototype doesn't have a replacement for PlainDagramSocketImpl at this time. There are a few issues around how legacy MulticastSocket behaves that will take a bit of time to sort out. Ideally its replacement will use the same infrastructure as the DatgramChannel implementation as that supports modern muilticasting features and is a lot easier to maintain. For now, we (= Michael McMahon and myself for now) will iterate on this prototype in the sandbox. If we go forward with it then we'll likely create a JEP. -Alan
[13] RFR (doc): 8217627: HttpClient: The API documentation of BodySubscribers::mapping promotes bad behavior
Hi, Please find below a doc clarification change for: 8217627: HttpClient: The API documentation of BodySubscribers::mapping promotes bad behavior https://bugs.openjdk.java.net/browse/JDK-8217627 webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8217627/webrev.00 New API doc: For better readability the new generated API documentation of BodySubscribers::mapping can be seen here: http://cr.openjdk.java.net/~dfuchs/webrev_8217627/webrev.00/api/java.net.http/java/net/http/HttpResponse.BodySubscribers.html#mapping(java.net.http.HttpResponse.BodySubscriber,java.util.function.Function) best regards, -- daniel
RFR [13] 8217429: WebSocket over authenticating proxy fails to send Upgrade headers
When tunneling WebSocket over a proxy requiring authentication, the implementation must ensure that the appropriate Upgrade headers are not lost after the tunnel has been established. The source changes are quite straight forward, the remaining bulk of the changes are to address a deficiency in the testing of proxying and authentication, when establishing a WebSocket connection. Webrev: http://cr.openjdk.java.net/~chegar/8217429/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8217429 -Chris
Re: [13] RFR (doc): 8217627: HttpClient: The API documentation of BodySubscribers::mapping promotes bad behavior
> On 25 Jan 2019, at 14:10, Daniel Fuchs wrote: > > Hi, > > Please find below a doc clarification change for: > > 8217627: HttpClient: The API documentation of BodySubscribers::mapping > promotes bad behavior > https://bugs.openjdk.java.net/browse/JDK-8217627 > > webrev: > http://cr.openjdk.java.net/~dfuchs/webrev_8217627/webrev.00 This looks good Daniel. -Chris.
Re: RFR [13] 8217429: WebSocket over authenticating proxy fails to send Upgrade headers
Chris, thanks for doing this! I have two questions on this change. 1. After this change has been applied, there will be a circular dependency between HttpRequestImpl and OpeningHandshake. If this code is used by these two classes maybe we are better off extracting it into some (already existing) third class? 2. Why does this change add server.close() to each and every test method of WebSocketTest? If I'm not mistaken that's what @AfterTest public void cleanup() is supposed to do. > On 25 Jan 2019, at 14:21, Chris Hegarty wrote: > > When tunneling WebSocket over a proxy requiring authentication, the > implementation must ensure that the appropriate Upgrade headers are > not lost after the tunnel has been established. The source changes are > quite straight forward, the remaining bulk of the changes are to address > a deficiency in the testing of proxying and authentication, when > establishing a WebSocket connection. > > Webrev: > http://cr.openjdk.java.net/~chegar/8217429/webrev.01/ > Bug: > https://bugs.openjdk.java.net/browse/JDK-8217429 > > -Chris
Re: RFR [13] 8217429: WebSocket over authenticating proxy fails to send Upgrade headers
Hi Chris, Looks good. I had the same question than Pavel about server.close(). No test for both proxy + server authorization with -Djdk.http.auth.tunneling.disabledSchemes ? cheers, -- daniel On 25/01/2019 14:21, Chris Hegarty wrote: When tunneling WebSocket over a proxy requiring authentication, the implementation must ensure that the appropriate Upgrade headers are not lost after the tunnel has been established. The source changes are quite straight forward, the remaining bulk of the changes are to address a deficiency in the testing of proxying and authentication, when establishing a WebSocket connection. Webrev: http://cr.openjdk.java.net/~chegar/8217429/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8217429 -Chris
Re: RFR [13] 8217429: WebSocket over authenticating proxy fails to send Upgrade headers
Pavel, > On 25 Jan 2019, at 15:16, Pavel Rappo wrote: > > Chris, thanks for doing this! I have two questions on this change. > > 1. After this change has been applied, there will be a circular dependency > between HttpRequestImpl and OpeningHandshake. If this code is used by these > two > classes maybe we are better off extracting it into some (already existing) > third > class? I moved the code to common.Utils, to avoid any unnecessary dependency. > 2. Why does this change add server.close() to each and every test method of > WebSocketTest? If I'm not mistaken that's what @AfterTest public void > cleanup() > is supposed to do. I think @AfterTest does not do what you think it does. @AfterTest: The annotated method will be run after all the test methods belonging to the classes inside the tag have run. Really, these tests should use try-with-resources, but I wanted avoid obfuscating the changes. That can be done separately. -Chris.
Re: RFR [13] 8217429: WebSocket over authenticating proxy fails to send Upgrade headers
Daniel, > On 25 Jan 2019, at 15:34, Daniel Fuchs wrote: > > Hi Chris, > > Looks good. > I had the same question than Pavel about server.close(). Answered already in reply to Pavel’s question. > No test for both proxy + server authorization with > -Djdk.http.auth.tunneling.disabledSchemes ? No, not yet. It's easy to add a test for that scenario, but there is a separate, kinda, unrelated issue that prevents it from working. Since fixing that issue requires changing code that will affect secure HTTP connections too, I prefer to separate that out into a different JIRA issue. -Chris.
Re: RFR [13] 8217429: WebSocket over authenticating proxy fails to send Upgrade headers
Thanks Chris. Makes sense, and looks good! best regards, -- daniel On 25/01/2019 17:07, Chris Hegarty wrote: Daniel, On 25 Jan 2019, at 15:34, Daniel Fuchs wrote: Hi Chris, Looks good. I had the same question than Pavel about server.close(). Answered already in reply to Pavel’s question. No test for both proxy + server authorization with -Djdk.http.auth.tunneling.disabledSchemes ? No, not yet. It's easy to add a test for that scenario, but there is a separate, kinda, unrelated issue that prevents it from working. Since fixing that issue requires changing code that will affect secure HTTP connections too, I prefer to separate that out into a different JIRA issue. -Chris.
Re: RFR [13] 8217429: WebSocket over authenticating proxy fails to send Upgrade headers
> On 25 Jan 2019, at 17:04, Chris Hegarty wrote: > > I moved the code to common.Utils, to avoid any unnecessary dependency. Thanks. >> 2. Why does this change add server.close() to each and every test method of >> WebSocketTest? If I'm not mistaken that's what @AfterTest public void >> cleanup() >> is supposed to do. > > I think @AfterTest does not do what you think it does. You are right, I was mistaken. I will have to make sure WebSocket test cases perform their cleanup correctly. Looks like it's not the only test file that has this issue. Try-with-resources might be okay and self-contained, true. However, for my liking it's a bit messy for this purpose. It's a duplication in every method and eats up one full indentation block. @AfterMethod [1] seems to be the right thing to do. [1] http://testng.org/doc/documentation-main.html#annotations
