Re: [8u] RFR(m): 8194298: Add support for per Socket configuration of TCP keepalive
Hi Bernd, When we added these socket options to JDK, that time these options were not available on Windows. Thanks, Vyom On Fri, Jun 26, 2020 at 7:12 AM Bernd Eckenfels wrote: > Hello, > > This would be a great addition. I do not understand why it does not > support the options available for Windows. Especially given the fact that > it actually implements 6 native methods to print "Unsupported". > > But I guess that's less a question to the backport and more to the general > implementation. > > Gruss > Bernd > > > -- > http://bernd.eckenfels.net > -- > *Von:* net-dev im Auftrag von Severin > Gehwolf > *Gesendet:* Thursday, June 25, 2020 3:39:38 PM > *An:* jdk8u-dev > *Cc:* net-dev > *Betreff:* [8u] RFR(m): 8194298: Add support for per Socket configuration > of TCP keepalive > > Hi, > > Please review this OpenJDK 8u backport of JDK-8194298 which adds socket > configuration options for TCP keepalive. It's one of the Oracle JDK > parity patches. > > As with the OpenJDK 11u change, this includes Linux and Mac only > changes. This backport is pretty much a rewrite as the JDK 11u codebase > is rather different in this area. > > Bug:https://bugs.openjdk.java.net/browse/JDK-8194298 > 8u CSR: https://bugs.openjdk.java.net/browse/JDK-8236187 > webrev: > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8194298/jdk8/04/webrev/ > > Testing: > * Build/test on Linux x86_64 and Mac OS x (thanks to Simon Tooke). >Build on Windows x86_64 (also thanks to Simon Tooke) > * tier1 tests, test/java/nio test/java/net/ test/jdk/net/ on Linux >x86_64. No regressions noted. > > Thoughts? > > Thanks, > Severin > > > -- Thanks, Vyom
Re: [8u] RFR(m): 8194298: Add support for per Socket configuration of TCP keepalive
Hi, On Thu, 2020-06-25 at 23:55 +, Bernd Eckenfels wrote: > This would be a great addition. Thanks. > I do not understand why it does not support the options available for > Windows. Especially given the fact that it actually implements 6 > native methods to print "Unsupported". > > But I guess that's less a question to the backport and more to the > general implementation. Yes, indeed. This should be brought up for discussion in JDK head first and implemented there before we can consider a backport. Thanks, Severin
Re: [8u] RFR(m): 8194298: Add support for per Socket configuration of TCP keepalive
Hi Severin, Overall code changes looks ok to me, I build & tested at my local REL it worked fine for me. Below are few minor comments. 1-> Net.java 1.1-> I think you don't need to explicitly convert value to integer and then pass it. You can avoid the local int variable creation as follows. ExtendedOptionsImpl.setTcpKeepAliveIntvl(fd, (Integer)value); 1.2-> In getSocketOption you don't need to explicitly cast it to Integer. return ExtendedOptionsImpl.getTcpKeepAliveIntvl(fd); 2-> PlainSocketImpl.java 1.1 -> In setOption(SocketOption name, T value) you can avoid the local int variable. 3-> Any specific reason for two set of functions "setTcpKeepAliveTime0 and setTcpKeepAliveTime" for all three socket options ? Thanks, Vyom On Fri, Jun 26, 2020 at 1:08 PM Severin Gehwolf wrote: > Hi, > > On Thu, 2020-06-25 at 23:55 +, Bernd Eckenfels wrote: > > This would be a great addition. > > Thanks. > > > I do not understand why it does not support the options available for > > Windows. Especially given the fact that it actually implements 6 > > native methods to print "Unsupported". > > > > But I guess that's less a question to the backport and more to the > > general implementation. > > Yes, indeed. This should be brought up for discussion in JDK head first > and implemented there before we can consider a backport. > > Thanks, > Severin > > > -- Thanks, Vyom
RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
Hello, Request to have my fix reviewed for issue: JDK-8247675 : WebSocket can loose the URL encoding of URI query parameters The fix updates the jdk.internal.net.http.websocket.OpeningHandshake to ensure that the URL is not reencoded/decoded and loose the original encoding Issue: https://bugs.openjdk.java.net/browse/JDK-8245245 webrev: http://cr.openjdk.java.net/~ryadav/webrev_8245245/index.html - rahul
RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
Hello, Request to have my fix reviewed for issue: JDK-8245245 : WebSocket can loose the URL encoding of URI query parameters The fix updates the jdk.internal.net.http.websocket.OpeningHandshake to ensure that the URL is not reencoded/decoded and loose the original encoding Issue: https://bugs.openjdk.java.net/browse/JDK-8245245 webrev: http://cr.openjdk.java.net/~ryadav/webrev_8245245/index.html - rahul
Re: RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
Rahul, Won't that start retaining the URL fragment? From https://tools.ietf.org/html/rfc6455#section-3 Fragment identifiers are meaningless in the context of WebSocket URIs and MUST NOT be used on these URIs. As with any URI scheme, the character "#", when not indicating the start of a fragment, MUST be escaped as %23. -Pavel > On 26 Jun 2020, at 13:03, Rahul Yadav wrote: > > Hello, > > Request to have my fix reviewed for issue: > > JDK-8245245 : WebSocket can loose the URL encoding of URI query parameters > > The fix updates the jdk.internal.net.http.websocket.OpeningHandshake > to ensure that the URL is not reencoded/decoded and loose the original > encoding > > Issue: https://bugs.openjdk.java.net/browse/JDK-8245245 > webrev: http://cr.openjdk.java.net/~ryadav/webrev_8245245/index.html > > - rahul
Re: RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
> On 26 Jun 2020, at 15:10, Pavel Rappo wrote: > > Chris, > > Currently (i.e. before the proposed change has been applied) the fragment is > NOT retained. This is because of the requirement of the mentioned RFC > section. The proposed change seems to overlook that detail. Hence, my > question. D’oh! Pavel is correct. I was reading the diffs the wrong way round ! -Chris.
Re: RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
Pavel,
That scenario is already handled, the existing behavior is if there is a
fragment, an exception is thrown.
That hasn't changed.
344 private static URI checkURI(URI uri) {
345 String scheme = uri.getScheme();
346 if (!("ws".equalsIgnoreCase(scheme) ||
"wss".equalsIgnoreCase(scheme)))
347 throw illegal("invalid URI scheme: " + scheme);
348 if (uri.getHost() == null)
349 throw illegal("URI must contain a host: " + uri);
350 if (uri.getFragment() != null)
351 throw illegal("URI must not contain a fragment: " + uri);
352 return uri;
353 }
354
- rahul
On 26/06/2020 14:38, Pavel Rappo wrote:
Rahul,
Won't that start retaining the URL fragment? From
https://tools.ietf.org/html/rfc6455#section-3
Fragment identifiers are meaningless in the context of WebSocket URIs
and MUST NOT be used on these URIs. As with any URI scheme, the
character "#", when not indicating the start of a fragment, MUST be
escaped as %23.
-Pavel
On 26 Jun 2020, at 13:03, Rahul Yadav wrote:
Hello,
Request to have my fix reviewed for issue:
JDK-8245245 : WebSocket can loose the URL encoding of URI query parameters
The fix updates the jdk.internal.net.http.websocket.OpeningHandshake
to ensure that the URL is not reencoded/decoded and loose the original
encoding
Issue: https://bugs.openjdk.java.net/browse/JDK-8245245
webrev: http://cr.openjdk.java.net/~ryadav/webrev_8245245/index.html
- rahul
Re: RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
> On 26 Jun 2020, at 14:38, Pavel Rappo wrote: > > Rahul, > > Won't that start retaining the URL fragment? From > https://tools.ietf.org/html/rfc6455#section-3 > > Fragment identifiers are meaningless in the context of WebSocket URIs > and MUST NOT be used on these URIs. As with any URI scheme, the > character "#", when not indicating the start of a fragment, MUST be > escaped as %23. I don’t think that a a fragment will be retained, see 182null); // No fragment , but maybe the concern is that a fragment character can be sneaked into other parts of the URI components, like the query? If so, then the test could be expanded to ensure that this cannot happen. -Chris.
Re: RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
I concur. Rahul has convinced me.
Rahul also pointed me to a test that verifies that the IAE is
thrown, so I believe that
http://cr.openjdk.java.net/~ryadav/webrev_8245245/index.html
is good.
best regards,
-- daniel
On 26/06/2020 16:42, Rahul Yadav wrote:
Pavel,
That scenario is already handled, the existing behavior is if there is a
fragment, an exception is thrown.
That hasn't changed.
344 private static URI checkURI(URI uri) {
345 String scheme = uri.getScheme();
346 if (!("ws".equalsIgnoreCase(scheme) ||
"wss".equalsIgnoreCase(scheme)))
347 throw illegal("invalid URI scheme: " + scheme);
348 if (uri.getHost() == null)
349 throw illegal("URI must contain a host: " + uri);
350 if (uri.getFragment() != null)
351 throw illegal("URI must not contain a fragment: " + uri);
352 return uri;
353 }
354
- rahul
Re: RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
Chris, Currently (i.e. before the proposed change has been applied) the fragment is NOT retained. This is because of the requirement of the mentioned RFC section. The proposed change seems to overlook that detail. Hence, my question. -Pavel > On 26 Jun 2020, at 14:51, Chris Hegarty wrote: > > > >> On 26 Jun 2020, at 14:38, Pavel Rappo wrote: >> >> Rahul, >> >> Won't that start retaining the URL fragment? From >> https://tools.ietf.org/html/rfc6455#section-3 >> >> Fragment identifiers are meaningless in the context of WebSocket URIs >> and MUST NOT be used on these URIs. As with any URI scheme, the >> character "#", when not indicating the start of a fragment, MUST be >> escaped as %23. > > I don’t think that a a fragment will be retained, see > > 182null); // No fragment > > , but maybe the concern is that a fragment character can be sneaked into > other parts of the URI components, like the query? If so, then the test > could be expanded to ensure that this cannot happen. > > -Chris. >
Re: RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
Looks good to me.
P.S. I think I began to forget the very code I wrote.
> On 26 Jun 2020, at 18:18, Daniel Fuchs wrote:
>
> I concur. Rahul has convinced me.
> Rahul also pointed me to a test that verifies that the IAE is
> thrown, so I believe that
> http://cr.openjdk.java.net/~ryadav/webrev_8245245/index.html
> is good.
>
> best regards,
>
> -- daniel
>
> On 26/06/2020 16:42, Rahul Yadav wrote:
>> Pavel,
>> That scenario is already handled, the existing behavior is if there is a
>> fragment, an exception is thrown.
>> That hasn't changed.
>> 344 private static URI checkURI(URI uri) {
>> 345 String scheme = uri.getScheme();
>> 346 if (!("ws".equalsIgnoreCase(scheme) ||
>> "wss".equalsIgnoreCase(scheme)))
>> 347 throw illegal("invalid URI scheme: " + scheme);
>> 348 if (uri.getHost() == null)
>> 349 throw illegal("URI must contain a host: " + uri);
>> 350 if (uri.getFragment() != null)
>> 351 throw illegal("URI must not contain a fragment: " + uri);
>> 352 return uri;
>> 353 }
>> 354
>> - rahul
Re: RFR 8237858: PlainSocketImpl.socketAccept() handles EINTR incorrectly
On 26/06/2020 05:17, Vyom Tiwari wrote: Hi Daniel/Alan, Please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8237858/webrev0.6/index.html). I think we can do better than one signal after 200ms. Have you given any thought to have a thread that signals many times so that you have a better chance of tickling bugs? One other comment at this time is that the value of SIGPIPE is still hardcoded - I think you'll need to add another native method to return its value. BTW: The formatting issues with linux_close.c and bsd_close.c are the same as the previous iterations. Did you track down that issue, I think you thought it was webrev or an issue with your patch queue. -Alan.
