Re: e Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers
Hi Anuuraag, Aleksei, Looks good to me. Except that now copyright years must be updated to 2020. No need for a new webrev if that's the only change! best regards, -- daniel On 09/01/2020 17:17, Aleks Efimov wrote: Hi Anuuraag, Latest webrev: http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/05/ Looks fine to me CI is also happy Best Regards, Aleksei
@since tag missing from DatagramSocket and MulticastSocket methods
Hi, Could someone please review my fix for JDK-8237075 '@since tag missing from DatagramSocket and MulticastSocket methods' ? This trivial fix adds in the missing @since tags for methods listed below. These methods were originally added in JDK 1.2. java/net/DatagramSocket: > public void connect(InetAddress address, int port) > public void disconnect() > public InetAddress getInetAddress() > public int getPort() > public synchronized void setSendBufferSize(int size) > public synchronized int getSendBufferSize() throws SocketException > public synchronized void setReceiveBufferSize(int size) > public synchronized int getReceiveBufferSize() java/net/MulticastSocket: > public void setTimeToLive(int ttl) > public int getTimeToLive() bug: https://bugs.openjdk.java.net/browse/JDK-8237075 webrev: http://cr.openjdk.java.net/~pconcannon/8237075/webrevs/webrev.00/ Kind regards, Patrick
Re: @since tag missing from DatagramSocket and MulticastSocket methods
On 14/01/2020 11:49, Patrick Concannon wrote: Hi, Could someone please review my fix for JDK-8237075 '@since tag missing from DatagramSocket and MulticastSocket methods' ? This trivial fix adds in the missing @since tags for methods listed below. These methods were originally added in JDK 1.2. Looks good and I've double checked that this is the right @since for these methods. -Alan
Re: @since tag missing from DatagramSocket and MulticastSocket methods
On 14/01/2020 11:49, Patrick Concannon wrote: bug: https://bugs.openjdk.java.net/browse/JDK-8237075 webrev: http://cr.openjdk.java.net/~pconcannon/8237075/webrevs/webrev.00/ Looks good to me as well. best regards, -- daniel
Re: @since tag missing from DatagramSocket and MulticastSocket methods
LGTM. Surprising that we haven’t noticed this before, because the @since tags are used, in part, to derive the symbol tables for --release ( maybe such old version information is not useful these days ) -Chris. > On 14 Jan 2020, at 11:49, Patrick Concannon > wrote: > > Hi, > > Could someone please review my fix for JDK-8237075 '@since tag missing from > DatagramSocket and MulticastSocket methods' ? > > This trivial fix adds in the missing @since tags for methods listed below. > These methods were originally added in JDK 1.2. > > java/net/DatagramSocket: > > > public void connect(InetAddress address, int port) > > public void disconnect() > > public InetAddress getInetAddress() > > public int getPort() > > public synchronized void setSendBufferSize(int size) > > public synchronized int getSendBufferSize() throws SocketException > > public synchronized void setReceiveBufferSize(int size) > > public synchronized int getReceiveBufferSize() > > java/net/MulticastSocket: > > > public void setTimeToLive(int ttl) > > public int getTimeToLive() > > > bug: https://bugs.openjdk.java.net/browse/JDK-8237075 > > webrev: http://cr.openjdk.java.net/~pconcannon/8237075/webrevs/webrev.00/ > > > Kind regards, > > Patrick >
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
Daniel, I imported the patch from the link you provided as follows: hg import --no-commit open.patch The patch applied successfully. I tried then to run the tests and saw that some of them could not be compiled. For instance, java/net/httpclient/websocket/BlowupOutputQueue.java java/net/httpclient/websocket/PendingPingBinaryClose.java ... etc. > On 13 Jan 2020, at 13:06, Daniel Fuchs wrote: > > Hi, > > Please find below a fix for: > 8236859: WebSocket over authenticating proxy fails with NPE > https://bugs.openjdk.java.net/browse/JDK-8236859 > > > http://cr.openjdk.java.net/~dfuchs/webrev_8236859/webrev.00/ > > It happens when we have a TLS tunnel and authentication with the server > fails: in that case the TLS tunnel is not established. The WS logic tries to > get the RawChannel to close it (as it should) but the debug trace gets in the > way and causes a NPE. > If the debug trace is fixed, the NPE occurs later (in connectFlows()). > The solution is to avoid creating the rawChannel for the sole purpose of > closing the connection. A new method added to RawChannel.Provider (internal > API) does the trick. > > While investigating this issue I stumbled over several other problems: > an assertion was occurring in the HTTP/1.1 connection pool, because > the failed connection was wrongly returned to the pool instead of > being closed. This will be fixed here two. > > Then I stumbled on another issue when both the proxy and the server > required credentials: the proxy credentials were not added to the cache > but requested every time because the 407 was followed by a 401, and > that caused 407 to be returned again at the next round trip, causing > the request to eventually fail with "too many retries". > This is now fixed in AuthenticationFilter. > > The WebSocketProxyTest.java is now updated to test wss: and to test > both proxy and server authentication in the same request. > > best regards, > > -- daniel
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
Hi Pavel, That's strange. Are you sure your hg import worked properly? Note that SecureSupport is a modified copy of Support (not a rename) and DummySecureWebSocketServer is a modified copy of DummyWebSocketServer (not a rename). I wonder if this was causing issue with the import. (the patch is obtained by hg diff, not hg export) best regards, -- daniel On 14/01/2020 14:39, Pavel Rappo wrote: Daniel, I imported the patch from the link you provided as follows: hg import --no-commit open.patch The patch applied successfully. I tried then to run the tests and saw that some of them could not be compiled. For instance, java/net/httpclient/websocket/BlowupOutputQueue.java java/net/httpclient/websocket/PendingPingBinaryClose.java ... etc. > On 13 Jan 2020, at 13:06, Daniel Fuchs wrote: >> http://cr.openjdk.java.net/~dfuchs/webrev_8236859/webrev.00/
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
That patch contains the following lines: --- old/test/jdk/java/net/httpclient/websocket/DummyWebSocketServer.java 2020-01-13 13:04:41.0 + +++ /dev/null 2020-01-13 13:04:41.0 + --- old/test/jdk/java/net/httpclient/websocket/Support.java 2020-01-13 13:04:44.0 + +++ /dev/null 2020-01-13 13:04:44.0 + If I'm reading this right, those two files are considered removed. Is it possible to produce the patch once again with, maybe using "--git" option this time? Thanks! > On 14 Jan 2020, at 15:14, Daniel Fuchs wrote: > > Hi Pavel, > > That's strange. Are you sure your hg import worked properly? > Note that SecureSupport is a modified copy of Support > (not a rename) and DummySecureWebSocketServer is a > modified copy of DummyWebSocketServer (not a rename). > > I wonder if this was causing issue with the import. > (the patch is obtained by hg diff, not hg export) > > best regards, > > -- daniel > > On 14/01/2020 14:39, Pavel Rappo wrote: >> Daniel, >> I imported the patch from the link you provided as follows: >> hg import --no-commit open.patch >> The patch applied successfully. I tried then to run the tests and saw that >> some >> of them could not be compiled. For instance, >> java/net/httpclient/websocket/BlowupOutputQueue.java >> java/net/httpclient/websocket/PendingPingBinaryClose.java >> ... >> etc. > > On 13 Jan 2020, at 13:06, Daniel Fuchs wrote: > >> http://cr.openjdk.java.net/~dfuchs/webrev_8236859/webrev.00/ >
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
On 14/01/2020 15:14, Daniel Fuchs wrote: I wonder if this was causing issue with the import. (the patch is obtained by hg diff, not hg export) Yes. Damn. the open.patch generated by webrev renames the files instead of copying/modifying... I have added a proper changeset generated with `hg export` http://cr.openjdk.java.net/~dfuchs/webrev_8236859/webrev.00/open.changeset.ws best regards, -- daniel
Re: e Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers
Thanks Daniel! Anuuraag, I will update the copyrights, no need to send new patch. Then will sponsor your change - commit and push to jdk/jdk. Best Regards, Aleksei On 14/01/2020 11:06, Daniel Fuchs wrote: Hi Anuuraag, Aleksei, Looks good to me. Except that now copyright years must be updated to 2020. No need for a new webrev if that's the only change! best regards, -- daniel On 09/01/2020 17:17, Aleks Efimov wrote: Hi Anuuraag, Latest webrev: http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/05/ Looks fine to me CI is also happy Best Regards, Aleksei
Re: 8236925: (dc) Upgrade DatagramChannel socket adaptor to extend MulticastSocket
Hi Alan, Globally this looks good to me. One small nit is that the `oldImpl` field could now also become final (by applying the same trick you did with createImpl - that is - have oldImpl() return a boolean rather than set the field and assign the result in the constructor. I've imported your patch and am currently running some tests. best regards, -- daniel On 10/01/2020 14:55, Alan Bateman wrote: This is a patch to "upgrade" the DatagramChannel socket adaptor to extend MulticastSocket. This is one of the final changes needed to DatagramChannel and its socket adaptor buddy to make it easy to replace the legacy DatagramSocket and MulticastSocket implementation. Daniel has a draft JEP [1] with the all the details on this effort. The changes should be straight-forward to review as most of the methods defined by MulticastSocket are easy to implement on DatagramChannel. A few review notes: 1. MulticastSocket does not have a protected constructor to create the socket with a custom DatagramSocketImpl. The DatagramSocketImpl mechanism is legacy and not worth adding a constructor now. So the most significant change in the patch is that the socket adaptor is changed to invoke the MulticastSocket(SocketAddress) constructor. This requires a special checked in the MulticastSocket and DatagramSocket constructor that isn't pretty but it has the advantage that the dummy DatagramSocketImpl can go away (it was never used anyway). 2. The DatagramSocket.impl field is changed to final. This is nice because it avoids accidents with lazily creating an impl (say where the socket adaptor doesn't override all methods). The final field means there are several opportunities to for cleanup but I don't want to go there in this patch (this is why getImpl continues to declare that it throws SocketException). I also don't want to complicate things for the JEP implementation in the sandbox. 3. The MulticastSocket spec in lacking in many areas and doesn't specify the exceptions or behavior for many scenarios. The joinGroup methods don't specify how they behave when already a member of the group. The leaveGroup method doesn't specify the behavior when not a member. Several methods don't specify the exception when invoked with null or other bad input. The overrides in the socket adaptor try to maintain compatibility with the the long standing behavior. 4. The DatagramSocket::socket spec is modified to drop the statement that the socket adaptor doesn't have any public methods beyond those defined by DatagramSocket. We need to drop the same statement from SocketChannel and ServerSocketChannel, something for another issue. So I need a CSR and that can also track the observable change that the returned object is a MulticastSocket. 5. One issue with the test is that is initially tickled an intermittent bug on macOS. The underlying setsockopt fails intermittently with ENOMEM when attempting to join the group. There have been sightings of this issue going back to JDK 7 [2]. Also sightings with the legacy MulticastSocket implementation. The patch has a temporary workaround to this issue. It's ugly but I can't duplicate it with this change. Daniel has been thinking about doing the same workaround while we figure out if there is a fix for macOS. The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/8236925/webrev/ -Alan [1] https://bugs.openjdk.java.net/browse/JDK-8235674 [2] https://mail.openjdk.java.net/pipermail/net-dev/2013-July/006769.html
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
That changeset applies fine, thanks. I was wondering what you had in mind when added OpeningHandshake:225. Was it for general robustness or you ran into something in particular? > On 14 Jan 2020, at 15:36, Daniel Fuchs wrote: > > On 14/01/2020 15:14, Daniel Fuchs wrote: >> I wonder if this was causing issue with the import. >> (the patch is obtained by hg diff, not hg export) > > Yes. Damn. the open.patch generated by webrev renames the files > instead of copying/modifying... > > I have added a proper changeset generated with `hg export` > http://cr.openjdk.java.net/~dfuchs/webrev_8236859/webrev.00/open.changeset.ws > > best regards, > > -- daniel >
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
Hi Pavel, On 14/01/2020 17:54, Pavel Rappo wrote: That changeset applies fine, thanks. I was wondering what you had in mind when added OpeningHandshake:225. Was it for general robustness or you ran into something in particular? More for general robustness. Sometimes assertion errors are fired when running tests. These are not exceptions. And sometimes a test gets stuck because it's using a proxy that's doing one connection at a time and waits fro the previous connection to be closed before accepting the next. But I didn't have to do this change to make something pass. best regards, -- daniel
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
I think the WebSocket part of the changeset look good to me. I have gone through the non-WebSocket part of the changes shallowly. I'm not an expert. > On 14 Jan 2020, at 17:59, Daniel Fuchs wrote: > > Hi Pavel, > > On 14/01/2020 17:54, Pavel Rappo wrote: >> That changeset applies fine, thanks. >> I was wondering what you had in mind when added OpeningHandshake:225. >> Was it for general robustness or you ran into something in particular? > > More for general robustness. Sometimes assertion errors are fired when > running tests. These are not exceptions. And sometimes a test gets > stuck because it's using a proxy that's doing one connection at a > time and waits fro the previous connection to be closed before > accepting the next. > > But I didn't have to do this change to make something pass. > > best regards, > > -- daniel >
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
On 14/01/2020 18:17, Pavel Rappo wrote: I think the WebSocket part of the changeset look good to me. I have gone through the non-WebSocket part of the changes shallowly. I'm not an expert. Thanks! Your review is appreciated! I will wait until Michael or Chris review the rest of the changes. best regards, -- daniel
Re: 8236925: (dc) Upgrade DatagramChannel socket adaptor to extend MulticastSocket
On 14/01/2020 17:49, Daniel Fuchs wrote: Hi Alan, Globally this looks good to me. One small nit is that the `oldImpl` field could now also become final (by applying the same trick you did with createImpl - that is - have oldImpl() return a boolean rather than set the field and assign the result in the constructor. Thanks for going through this. I tried to keep the changes to DatagramSocket to a minimum but I don't mind making an exception for oldImpl. It's a slippery slope as there is a lot of technical debt in this area. Also oldImpl is the the support for DatagramSocketImpl that were compiled for JDK 1.3 or older and I really hope we can drop that code soon. Here's the updated webrev that changes oldImpl to be final. http://cr.openjdk.java.net/~alanb/8236925/2/webrev/ -Alan
Re: 8236925: (dc) Upgrade DatagramChannel socket adaptor to extend MulticastSocket
Looks good Alan! best regards. -- daniel On 14/01/20 19:23, Alan Bateman wrote: Thanks for going through this. I tried to keep the changes to DatagramSocket to a minimum but I don't mind making an exception for oldImpl. It's a slippery slope as there is a lot of technical debt in this area. Also oldImpl is the the support for DatagramSocketImpl that were compiled for JDK 1.3 or older and I really hope we can drop that code soon. Here's the updated webrev that changes oldImpl to be final. http://cr.openjdk.java.net/~alanb/8236925/2/webrev/ -Alan
Re: e Re: [PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers
Hi Aleksei, Daniel, Great news, thanks a lot for all your help on cleaning up this patch! Cheers, - Anuraag On Wed, Jan 15, 2020, 01:14 Aleks Efimov wrote: > Thanks Daniel! > > Anuuraag, > I will update the copyrights, no need to send new patch. Then will > sponsor your change - commit and push to jdk/jdk. > > Best Regards, > Aleksei > > > > On 14/01/2020 11:06, Daniel Fuchs wrote: > > Hi Anuuraag, Aleksei, > > > > Looks good to me. > > > > Except that now copyright years must be updated to 2020. > > No need for a new webrev if that's the only change! > > > > best regards, > > > > -- daniel > > > > On 09/01/2020 17:17, Aleks Efimov wrote: > >> Hi Anuuraag, > >> > >> Latest webrev: http://cr.openjdk.java.net/~aefimov/anuraaga/7006496/05/ > >> Looks fine to me > >> CI is also happy > >> > >> Best Regards, > >> Aleksei > > > >
