Integrated: 8286873: Improve websocket test execution time
On Tue, 17 May 2022 12:45:52 GMT, Daniel Jeliński wrote: > This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. This pull request has now been integrated. Changeset: d24c84e7 Author:Daniel Jeliński URL: https://git.openjdk.java.net/jdk/commit/d24c84e7687890db88550b05ff9eebe9cae361b2 Stats: 140 lines in 10 files changed: 68 ins; 39 del; 33 mod 8286873: Improve websocket test execution time Reviewed-by: dfuchs, prappo - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v3]
On Thu, 19 May 2022 12:05:45 GMT, Daniel Jeliński wrote: >> This PR improves the execution time of jdk_net tests (and, by extension, >> tier2) by about 3 minutes. >> >> Tests located under `jdk/java/net/httpclient/websocket` are never run in >> parallel. Each of the 8 modified `Pending***` tests originally required 40 >> seconds to complete. After the proposed changes, they usually complete in 15 >> seconds. >> >> This PR modifies the tests to initially run with 1 second timeout. If the >> test fails with 1 second timeout, it is retried with timeout increased to 10 >> seconds (the original value). >> >> The modified tests were executed at least 10 times on each of: Windows, >> Linux (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were >> observed. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Make it clear that both actions hang Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v3]
On Thu, 19 May 2022 10:46:35 GMT, Pavel Rappo wrote: > > > What looks questionable is rearrangement of asserts: when `assertHangs` > > > moves down. assertNotDone(cfClose) can transitorry pass even if ping has > > > not hung. > > > > > > `assertHangs` either waits for 5 seconds or throws an exception, so > > `assertNotDone` after `assertHangs` is pretty much equivalent to > > `assertHangs` > > The order of these checks is important. On the other hand, I'm not sure how > important that importance is for the tests. With the proposed change, the > test might become more permissive than it should've been. > > Separately, why does this PR change `assertHangs(cfClose)` to > `assertNotDone(cfClose)`? > > The fact that you say that the tests still pass gives me some confidence. I'm > just trying to make sure that the tests still test what we think they test. Modified the tests to make it clear that we expect both CFs to hang. Let me know if that addresses your concerns. - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v3]
> This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Make it clear that both actions hang - Changes: - all: https://git.openjdk.java.net/jdk/pull/8746/files - new: https://git.openjdk.java.net/jdk/pull/8746/files/bd7d3298..832d7f9a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=01-02 Stats: 24 lines in 9 files changed: 6 ins; 9 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8746.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8746/head:pull/8746 PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v2]
On Thu, 19 May 2022 09:42:05 GMT, Daniel Jeliński wrote: > > What looks questionable is rearrangement of asserts: when `assertHangs` > > moves down. assertNotDone(cfClose) can transitorry pass even if ping has > > not hung. > > `assertHangs` either waits for 5 seconds or throws an exception, so > `assertNotDone` after `assertHangs` is pretty much equivalent to `assertHangs` The order of these checks is important. On the other hand, I'm not sure how important that importance is for the tests. With the proposed change, the test might become more permissive than it should've been. Separately, why does this PR change `assertHangs(cfClose)` to `assertNotDone(cfClose)`? The fact that you say that the tests still pass gives me some confidence. I'm just trying to make sure that the tests still test what we think they test. - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v2]
> This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Fix wording - Changes: - all: https://git.openjdk.java.net/jdk/pull/8746/files - new: https://git.openjdk.java.net/jdk/pull/8746/files/52b735bf..bd7d3298 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8746.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8746/head:pull/8746 PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time
On Thu, 19 May 2022 09:22:57 GMT, Pavel Rappo wrote: > What looks questionable is rearrangement of asserts: when `assertHangs` moves > down. assertNotDone(cfClose) can transitorry pass even if ping has not hung. `assertHangs` either waits for 5 seconds or throws an exception, so `assertNotDone` after `assertHangs` is pretty much equivalent to `assertHangs` - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time
On Tue, 17 May 2022 14:55:11 GMT, Daniel Fuchs wrote: > I am a bit less sure about moving the post-asserts inside the loop I moved them because they too can fail if the original blocked future suddenly completes. Side effect of this change is that any failures that happen after websocket.abort will be retried (isDone will return true). I think this is acceptable. > or closing before asserting that the cf hangs, but if I understand the logic > correctly it seems ok too. `assertHangs` essentially just waits for five seconds to see if the future finishes in the meantime. With the proposed change we are waiting for 2 futures at the same time. - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time
On Tue, 17 May 2022 12:45:52 GMT, Daniel Jeliński wrote: > This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. I have poor recollection of the mechanics of these tests. The source of wait is timed get and `assertHang`. You are reducing the former. That looks good. What looks questionable is rearrangement of asserts: when `assertHangs` moves down. assertNotDone(cfClose) can transitorry pass even if ping has not hung. Tiriviality: since you are here, please remove 1. superfluous "are" on BlowupOutputQueue:60 2. "the a" on BlowupOutputQueue:63 test/jdk/java/net/httpclient/websocket/PendingOperations.java line 44: > 42: static final Class IOE = IOException.class; > 43: // Time after which we deem that the local send buffer and remote > 44: // receive buffer must be full. This has been heuristically > determined. "Heuristically" was a weird word to use here; should've been "empirically", I think. You deleted the whole sentence, which is okay too. - Changes requested by prappo (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time
On Tue, 17 May 2022 12:45:52 GMT, Daniel Jeliński wrote: > This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. Looks ok-ish. The idea of starting with a small timeout is a good one. I am a bit less sure about moving the post-asserts inside the loop - or closing before asserting that the cf hangs, but if I understand the logic correctly it seems ok too. Maybe give some time for @pavelrappo to chime in before integrating. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8746
RFR: 8286873: Improve websocket test execution time
This PR improves the execution time of jdk_net tests (and, by extension, tier2) by about 3 minutes. Tests located under `jdk/java/net/httpclient/websocket` are never run in parallel. Each of the 8 modified `Pending***` tests originally required 40 seconds to complete. After the proposed changes, they usually complete in 15 seconds. This PR modifies the tests to initially run with 1 second timeout. If the test fails with 1 second timeout, it is retried with timeout increased to 10 seconds (the original value). The modified tests were executed at least 10 times on each of: Windows, Linux (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were observed. - Commit messages: - Explain magic timeouts - Retry all operations - Increase initial wait for Windows machines - Fix test retry - Avoid one 5-second wait - Use shorter timeout on the first try Changes: https://git.openjdk.java.net/jdk/pull/8746/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286873 Stats: 139 lines in 10 files changed: 70 ins; 38 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/8746.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8746/head:pull/8746 PR: https://git.openjdk.java.net/jdk/pull/8746
[jdk17] Integrated: 8268714: [macos-aarch64] 7 java/net/httpclient/websocket tests failed
On Wed, 16 Jun 2021 13:53:38 GMT, Daniel Fuchs wrote:
> Hi,
>
> Please find below a test-only change to fix some intermittent failures
> observed with the httpclient/websocket tests:
> these tests intermittently and randomly fail with ENOMEM ("No buffer space
> available").
>
> Some machines in our CI seem to allow a higher level of concurrency while
> being (maybe) configured with lower system resources (such as available
> buffer space for the TCP stack).
>
> Some of the httpclient/websocket tests attempt to fill the sockets buffers in
> order to assert some conditions when the buffers are full and writing is
> paused. When the test process terminates, this leaves behind TCP sockets in
> the TIME_WAIT state that still hold system buffer resources in case
> retransmission is needed. When several such tests are run this ends up
> causing random "No buffer space available" errors on other tests (including
> these tests themselves) running concurrently or shortly after on the same
> machine.
>
> This change implements a few tricks to alleviate the situation:
> - configure the tests with smaller send buffers on the client side and
> receive buffers on the server side, in order to limit how much buffer space
> is consumed by the test.
> - when the not-reading server is closed, and before the accepted socket is
> closed, read all available data off the socket buffer in order to free up the
> buffer space that the test has consumed before closing the socket.
> - in some tests that create a large number of HttpClients, limit the number
> of clients created in shared client mode, and add a call to System.gc() and a
> small pause to give time for gc to collect the old clients which are no
> longer referenced.
>
> With these changes, I have run the HttpClient tests 200 times on the
> problematic machines without observing any failures (where previously there
> was at least a couple of failures per 50 runs). I also ran tier1 once, and
> tier2 twice and the results came clean.
>
> I am therefore claiming success (even if it might prove temporary ;-) )
>
> If these failures come back to haunt the CI again after this fix, a further
> remediation policy could be to put the httpclient/websocket directory in
> exclusive test execution mode (in TEST.root) - this seems to work too - but
> cleaning up garbage in the tests themselves seems preferable.
This pull request has now been integrated.
Changeset: 8ea0606a
Author:Daniel Fuchs
URL:
https://git.openjdk.java.net/jdk17/commit/8ea0606aba15911f5bfe2c81a83b42288d97095f
Stats: 93 lines in 12 files changed: 86 ins; 0 del; 7 mod
8268714: [macos-aarch64] 7 java/net/httpclient/websocket tests failed
Reviewed-by: chegar, michaelm
-
PR: https://git.openjdk.java.net/jdk17/pull/79
Re: [jdk17] RFR: 8268714: [macos-aarch64] 7 java/net/httpclient/websocket tests failed
On Wed, 16 Jun 2021 13:53:38 GMT, Daniel Fuchs wrote:
> Hi,
>
> Please find below a test-only change to fix some intermittent failures
> observed with the httpclient/websocket tests:
> these tests intermittently and randomly fail with ENOMEM ("No buffer space
> available").
>
> Some machines in our CI seem to allow a higher level of concurrency while
> being (maybe) configured with lower system resources (such as available
> buffer space for the TCP stack).
>
> Some of the httpclient/websocket tests attempt to fill the sockets buffers in
> order to assert some conditions when the buffers are full and writing is
> paused. When the test process terminates, this leaves behind TCP sockets in
> the TIME_WAIT state that still hold system buffer resources in case
> retransmission is needed. When several such tests are run this ends up
> causing random "No buffer space available" errors on other tests (including
> these tests themselves) running concurrently or shortly after on the same
> machine.
>
> This change implements a few tricks to alleviate the situation:
> - configure the tests with smaller send buffers on the client side and
> receive buffers on the server side, in order to limit how much buffer space
> is consumed by the test.
> - when the not-reading server is closed, and before the accepted socket is
> closed, read all available data off the socket buffer in order to free up the
> buffer space that the test has consumed before closing the socket.
> - in some tests that create a large number of HttpClients, limit the number
> of clients created in shared client mode, and add a call to System.gc() and a
> small pause to give time for gc to collect the old clients which are no
> longer referenced.
>
> With these changes, I have run the HttpClient tests 200 times on the
> problematic machines without observing any failures (where previously there
> was at least a couple of failures per 50 runs). I also ran tier1 once, and
> tier2 twice and the results came clean.
>
> I am therefore claiming success (even if it might prove temporary ;-) )
>
> If these failures come back to haunt the CI again after this fix, a further
> remediation policy could be to put the httpclient/websocket directory in
> exclusive test execution mode (in TEST.root) - this seems to work too - but
> cleaning up garbage in the tests themselves seems preferable.
LGTM
-
Marked as reviewed by michaelm (Reviewer).
PR: https://git.openjdk.java.net/jdk17/pull/79
Re: [jdk17] RFR: 8268714: [macos-aarch64] 7 java/net/httpclient/websocket tests failed
On Wed, 16 Jun 2021 13:53:38 GMT, Daniel Fuchs wrote:
> Hi,
>
> Please find below a test-only change to fix some intermittent failures
> observed with the httpclient/websocket tests:
> these tests intermittently and randomly fail with ENOMEM ("No buffer space
> available").
>
> Some machines in our CI seem to allow a higher level of concurrency while
> being (maybe) configured with lower system resources (such as available
> buffer space for the TCP stack).
>
> Some of the httpclient/websocket tests attempt to fill the sockets buffers in
> order to assert some conditions when the buffers are full and writing is
> paused. When the test process terminates, this leaves behind TCP sockets in
> the TIME_WAIT state that still hold system buffer resources in case
> retransmission is needed. When several such tests are run this ends up
> causing random "No buffer space available" errors on other tests (including
> these tests themselves) running concurrently or shortly after on the same
> machine.
>
> This change implements a few tricks to alleviate the situation:
> - configure the tests with smaller send buffers on the client side and
> receive buffers on the server side, in order to limit how much buffer space
> is consumed by the test.
> - when the not-reading server is closed, and before the accepted socket is
> closed, read all available data off the socket buffer in order to free up the
> buffer space that the test has consumed before closing the socket.
> - in some tests that create a large number of HttpClients, limit the number
> of clients created in shared client mode, and add a call to System.gc() and a
> small pause to give time for gc to collect the old clients which are no
> longer referenced.
>
> With these changes, I have run the HttpClient tests 200 times on the
> problematic machines without observing any failures (where previously there
> was at least a couple of failures per 50 runs). I also ran tier1 once, and
> tier2 twice and the results came clean.
>
> I am therefore claiming success (even if it might prove temporary ;-) )
>
> If these failures come back to haunt the CI again after this fix, a further
> remediation policy could be to put the httpclient/websocket directory in
> exclusive test execution mode (in TEST.root) - this seems to work too - but
> cleaning up garbage in the tests themselves seems preferable.
Looks good to me. ( a pragmatic approach to the issue at hand - resolve
intermittently failing tests )
-
Marked as reviewed by chegar (Reviewer).
PR: https://git.openjdk.java.net/jdk17/pull/79
[jdk17] RFR: 8268714: [macos-aarch64] 7 java/net/httpclient/websocket tests failed
Hi,
Please find below a test-only change to fix some intermittent failures observed
with the httpclient/websocket tests:
these tests intermittently and randomly fail with ENOMEM ("No buffer space
available").
Some machines in our CI seem to allow a higher level of concurrency while being
(maybe) configured with lower system resources (such as available buffer space
for the TCP stack).
Some of the httpclient/websocket tests attempt to fill the sockets buffers in
order to assert some conditions when the buffers are full and writing is
paused. When the test process terminates, this leaves behind TCP sockets in the
TIME_WAIT state that still hold system buffer resources in case retransmission
is needed. When several such tests are run this ends up causing random "No
buffer space available" errors on other tests (including these tests
themselves) running concurrently or shortly after on the same machine.
This change implements a few tricks to alleviate the situation:
- configure the tests with smaller send buffers on the client side and receive
buffers on the server side, in order to limit how much buffer space is consumed
by the test.
- when the not-reading server is closed, and before the accepted socket is
closed, read all available data off the socket buffer in order to free up the
buffer space that the test has consumed before closing the socket.
- in some tests that create a large number of HttpClients, limit the number of
clients created in shared client mode, and add a call to System.gc() and a
small pause to give time for gc to collect the old clients which are no longer
referenced.
With these changes, I have run the HttpClient tests 200 times on the
problematic machines without observing any failures (where previously there was
at least a couple of failures per 50 runs). I also ran tier1 once, and tier2
twice and the results came clean.
I am therefore claiming success (even if it might prove temporary ;-) )
If these failures come back to haunt the CI again after this fix, a further
remediation policy could be to put the httpclient/websocket directory in
exclusive test execution mode (in TEST.root) - this seems to work too - but
cleaning up garbage in the tests themselves seems preferable.
-
Commit messages:
- 8268714: [macos-aarch64] 7 java/net/httpclient/websocket tests failed
Changes: https://git.openjdk.java.net/jdk17/pull/79/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=79&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8268714
Stats: 93 lines in 12 files changed: 86 ins; 0 del; 7 mod
Patch: https://git.openjdk.java.net/jdk17/pull/79.diff
Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/79/head:pull/79
PR: https://git.openjdk.java.net/jdk17/pull/79
Re: Unix Domain Socket with Websocket
Thank you all for your replies. Kind regards -Christos On Fri, Jun 11, 2021 at 12:50 PM Michael McMahon < michael.x.mcma...@oracle.com> wrote: > Yes, it does seem that for local RPC a regular (unix domain) socket should > suffice > rather than a websocket. > > Also, just to point out that Unix domain socket (channels) have been in > the JDK > since Java 16. > > - Michael. > On 11/06/2021 10:22, Daniel Fuchs wrote: > > Hi Christos, > > The HttpClient doesn't support Unix Domain Socket - only regular > TCP / TLS. > > > You could of course open a connection with your client using > a plain Unix Domain SocketChannel [1] using the UNIX > ProtocolFamilly [2]. > > best regard, > > -- daniel > [1] > https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/nio/channels/SocketChannel.html#open(java.net.ProtocolFamily) > [2] > https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/net/StandardProtocolFamily.html > > On 11/06/2021 09:57, Christos Vasilakis wrote: > > Hello all, > > we've a native application (in C) that exposes a json-rpc interface over a > Unix Domain Socket using websocket. I know that the upcoming Java 17 > release will include support for unix domain sockets and I'm wondering if > the included websocket client (since Java 11) will also support this > mechanism ? > > If not planned, any advice on how you can achieve this today ? > > * me hopes that I don't need to touch C code and continue with Java :-) > > Kind regards, > -Christos > > > > >
Re: Unix Domain Socket with Websocket
Yes, it does seem that for local RPC a regular (unix domain) socket should suffice rather than a websocket. Also, just to point out that Unix domain socket (channels) have been in the JDK since Java 16. - Michael. On 11/06/2021 10:22, Daniel Fuchs wrote: Hi Christos, The HttpClient doesn't support Unix Domain Socket - only regular TCP / TLS. You could of course open a connection with your client using a plain Unix Domain SocketChannel [1] using the UNIX ProtocolFamilly [2]. best regard, -- daniel [1] https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/nio/channels/SocketChannel.html#open(java.net.ProtocolFamily) [2] https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/net/StandardProtocolFamily.html On 11/06/2021 09:57, Christos Vasilakis wrote: Hello all, we've a native application (in C) that exposes a json-rpc interface over a Unix Domain Socket using websocket. I know that the upcoming Java 17 release will include support for unix domain sockets and I'm wondering if the included websocket client (since Java 11) will also support this mechanism ? If not planned, any advice on how you can achieve this today ? * me hopes that I don't need to touch C code and continue with Java :-) Kind regards, -Christos
Re: Unix Domain Socket with Websocket
Hi Christos, The HttpClient doesn't support Unix Domain Socket - only regular TCP / TLS. You could of course open a connection with your client using a plain Unix Domain SocketChannel [1] using the UNIX ProtocolFamilly [2]. best regard, -- daniel [1] https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/nio/channels/SocketChannel.html#open(java.net.ProtocolFamily) [2] https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/net/StandardProtocolFamily.html On 11/06/2021 09:57, Christos Vasilakis wrote: Hello all, we've a native application (in C) that exposes a json-rpc interface over a Unix Domain Socket using websocket. I know that the upcoming Java 17 release will include support for unix domain sockets and I'm wondering if the included websocket client (since Java 11) will also support this mechanism ? If not planned, any advice on how you can achieve this today ? * me hopes that I don't need to touch C code and continue with Java :-) Kind regards, -Christos
Unix Domain Socket with Websocket
Hello all, we've a native application (in C) that exposes a json-rpc interface over a Unix Domain Socket using websocket. I know that the upcoming Java 17 release will include support for unix domain sockets and I'm wondering if the included websocket client (since Java 11) will also support this mechanism ? If not planned, any advice on how you can achieve this today ? * me hopes that I don't need to touch C code and continue with Java :-) Kind regards, -Christos
Integrated: 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with "IOException: No buffer space available"
On Thu, 27 May 2021 09:32:06 GMT, Daniel Fuchs wrote: > Hi, > > Please find below a fix for: > 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with > "IOException: No buffer space available" > > The Pending* websocket tests create a server that accepts sockets to create a > websocket, but never read data from the websocket in order to get the client > side to block once the buffers are full. > Unfortunately, the PendingOperations:cleanup methods was not called after > each test methods invocations, causing sockets to leak and relying on the gc > to cleanup. > > This caused an "IOException: No buffer space available" to be raised > intermittently but reliably (1 run out of 50 in PendingTextPingClose). > > The fix makes sure that cleanup() is called appropriately after each test > method invocation. > It also tweaks PendingTextPingClose - which seemed to be failing more > frequently - to use smaller send and receive buffers in order to reach the > point at which the client bocks more quickly and avoid wasting system > resources. This pull request has now been integrated. Changeset: 24bf35f8 Author:Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/24bf35f862e285eeca662b9829901c0f91d247d5 Stats: 138 lines in 14 files changed: 83 ins; 24 del; 31 mod 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with "IOException: No buffer space available" Reviewed-by: chegar - PR: https://git.openjdk.java.net/jdk/pull/4222
Re: RFR: 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with "IOException: No buffer space available" [v2]
On Thu, 27 May 2021 13:20:26 GMT, Daniel Fuchs wrote: >> Hi, >> >> Please find below a fix for: >> 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with >> "IOException: No buffer space available" >> >> The Pending* websocket tests create a server that accepts sockets to create >> a websocket, but never read data from the websocket in order to get the >> client side to block once the buffers are full. >> Unfortunately, the PendingOperations:cleanup methods was not called after >> each test methods invocations, causing sockets to leak and relying on the gc >> to cleanup. >> >> This caused an "IOException: No buffer space available" to be raised >> intermittently but reliably (1 run out of 50 in PendingTextPingClose). >> >> The fix makes sure that cleanup() is called appropriately after each test >> method invocation. >> It also tweaks PendingTextPingClose - which seemed to be failing more >> frequently - to use smaller send and receive buffers in order to reach the >> point at which the client bocks more quickly and avoid wasting system >> resources. > > Daniel Fuchs has updated the pull request incrementally with two additional > commits since the last revision: > > - Added missing spaces after catch > - Added missing brace in comment Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4222
Re: RFR: 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with "IOException: No buffer space available" [v2]
> Hi, > > Please find below a fix for: > 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with > "IOException: No buffer space available" > > The Pending* websocket tests create a server that accepts sockets to create a > websocket, but never read data from the websocket in order to get the client > side to block once the buffers are full. > Unfortunately, the PendingOperations:cleanup methods was not called after > each test methods invocations, causing sockets to leak and relying on the gc > to cleanup. > > This caused an "IOException: No buffer space available" to be raised > intermittently but reliably (1 run out of 50 in PendingTextPingClose). > > The fix makes sure that cleanup() is called appropriately after each test > method invocation. > It also tweaks PendingTextPingClose - which seemed to be failing more > frequently - to use smaller send and receive buffers in order to reach the > point at which the client bocks more quickly and avoid wasting system > resources. Daniel Fuchs has updated the pull request incrementally with two additional commits since the last revision: - Added missing spaces after catch - Added missing brace in comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/4222/files - new: https://git.openjdk.java.net/jdk/pull/4222/files/268e54e6..94c44add Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4222&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4222&range=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/4222.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4222/head:pull/4222 PR: https://git.openjdk.java.net/jdk/pull/4222
RFR: 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with "IOException: No buffer space available"
Hi, Please find below a fix for: 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with "IOException: No buffer space available" The Pending* websocket tests create a server that accepts sockets to create a websocket, but never read data from the websocket in order to get the client side to block once the buffers are full. Unfortunately, the PendingOperations:cleanup methods was not called after each test methods invocations, causing sockets to leak and relying on the gc to cleanup. This caused an "IOException: No buffer space available" to be raised intermittently but reliably (1 run out of 50 in PendingTextPingClose). The fix makes sure that cleanup() is called appropriately after each test method invocation. It also tweaks PendingTextPingClose - which seemed to be failing more frequently - to use smaller send and receive buffers in order to reach the point at which the client bocks more quickly and avoid wasting system resources. - Commit messages: - 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with "IOException: No buffer space available" Changes: https://git.openjdk.java.net/jdk/pull/4222/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4222&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265367 Stats: 135 lines in 14 files changed: 83 ins; 24 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/4222.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4222/head:pull/4222 PR: https://git.openjdk.java.net/jdk/pull/4222
Re: 8249786: java/net/httpclient/websocket/PendingPingTextClose.java fails very infrequently
Hi Pavel, Thanks for the review! On 07/08/2020 13:49, Pavel Rappo wrote: May I suggest we use this? ChannelState s; assert (s = writeState.get()) == CLOSED : s; Or better still, ChannelState s = writeState.get(); assert s == CLOSED : s; I'll do that before pushing. best regards, -- daniel
Re: 8249786: java/net/httpclient/websocket/PendingPingTextClose.java fails very infrequently
Daniel, The changes in TransportImpl look okay to me. I cannot see how they break anything. On the other hand, I guess it will take some time to see if the (complete) changeset fixes the issue. One minor thing that makes me a tad bit uncomfortable is the new assert statements. Firstly, they may have visibility side effects. Secondly, they could be more informative. May I suggest we use this? ChannelState s; assert (s = writeState.get()) == CLOSED : s; Or better still, ChannelState s = writeState.get(); assert s == CLOSED : s; -Pavel > On 23 Jul 2020, at 16:02, Daniel Fuchs wrote: > > Hi, > > More testing revealed that some other tests of the same family > kept on failing intermittently, though my changes to > PendingOperation.java should have fixed them. > > So here is a broader fix - which seems to have fixed the issue. > But as a consequence - I am no longer planning to push it to > 15 as it also changes some source files: > > http://cr.openjdk.java.net/~dfuchs/webrev_8249786/webrev.02/ > > best regards, > > -- daniel > > On 21/07/2020 18:53, Daniel Fuchs wrote: >> Hi, >> Please find below a fix for: >> 8249786: java/net/httpclient/websocket/PendingPingTextClose.java >> fails very infrequently >> https://bugs.openjdk.java.net/browse/JDK-8249786 >> webrev: >> http://cr.openjdk.java.net/~dfuchs/webrev_8249786/webrev.00/ >> This test has been observed failing on windows in the JDK 15 CI. >> The most troublesome issue is that the test was producing so >> much output that the actual reason for the failure was lost >> in the output overflow. >> After instrumenting the test to limit the output and >> adding a few higher level traces, I was able to reproduce >> once (out of 250 runs) and see the actual stack trace. >> The test fails just after calling websocket.abort() when it tries >> to verify that the cfPing CompletableFuture completed >> exceptionally, and found that it actually successfully completed. >> The logic of the test is to try to fill up the local send buffer >> by sending ping messages, so that an attempt to write to the >> socket will block. >> For that it creates a server that will accept a websocket >> connection, but will not read anything from the socket input. >> The client side sends ping packets until the socket buffer >> is full - which is detected by setting up a 10s timeout and >> observing that the ping data could not be written during >> this time. The assumption of the test is that a write call >> that takes more than 10s is indicative that the buffers are >> full, and will never succeed. >> The problem occurs when the write succeeds after ~10s either >> because the kernel was busy doing some other things, or because >> the kernel suddenly decided to resize (increase) the buffers, >> which causes the write call to unblock and succeed after 10s. >> The test already had some provision and a workaround for that >> issue - via a repeatable( ) operation - but the workaround >> was only enabled for macOS where such behavior had first been >> observed. >> The fix extends that workaround for Windows - since the later >> failure shows that something similar is happening there. >> The fix also moves the websocket.abort() and following check >> inside the repeatable loop for better reliability. >> Since pushing test fixes during rampdown 2 is permitted, >> and since the failure was observed in the JDK 15 CI, I'm >> planning to push this test fix to the jdk15 repo, >> unless I hear any objection. >> best regards, >> -- daniel >
Re: 8249786: java/net/httpclient/websocket/PendingPingTextClose.java fails very infrequently
Thanks Chris! best regards, -- daniel On 07/08/2020 09:44, Chris Hegarty wrote: http://cr.openjdk.java.net/~dfuchs/webrev_8249786/webrev.02/ LGTM - extending repeatable to cover Windows platforms too. ( the previous problems with these tests came flooding back when reading the comment in PendingOperations! )
Re: 8249786: java/net/httpclient/websocket/PendingPingTextClose.java fails very infrequently
Daniel, > On 23 Jul 2020, at 16:02, Daniel Fuchs wrote: > > Hi, > > More testing revealed that some other tests of the same family > kept on failing intermittently, though my changes to > PendingOperation.java should have fixed them. > > So here is a broader fix - which seems to have fixed the issue. > But as a consequence - I am no longer planning to push it to > 15 as it also changes some source files: > > http://cr.openjdk.java.net/~dfuchs/webrev_8249786/webrev.02/ LGTM - extending repeatable to cover Windows platforms too. ( the previous problems with these tests came flooding back when reading the comment in PendingOperations! ) -Chris.
8249786: java/net/httpclient/websocket/PendingPingTextClose.java fails very infrequently
Hi, More testing revealed that some other tests of the same family kept on failing intermittently, though my changes to PendingOperation.java should have fixed them. So here is a broader fix - which seems to have fixed the issue. But as a consequence - I am no longer planning to push it to 15 as it also changes some source files: http://cr.openjdk.java.net/~dfuchs/webrev_8249786/webrev.02/ best regards, -- daniel On 21/07/2020 18:53, Daniel Fuchs wrote: Hi, Please find below a fix for: 8249786: java/net/httpclient/websocket/PendingPingTextClose.java fails very infrequently https://bugs.openjdk.java.net/browse/JDK-8249786 webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8249786/webrev.00/ This test has been observed failing on windows in the JDK 15 CI. The most troublesome issue is that the test was producing so much output that the actual reason for the failure was lost in the output overflow. After instrumenting the test to limit the output and adding a few higher level traces, I was able to reproduce once (out of 250 runs) and see the actual stack trace. The test fails just after calling websocket.abort() when it tries to verify that the cfPing CompletableFuture completed exceptionally, and found that it actually successfully completed. The logic of the test is to try to fill up the local send buffer by sending ping messages, so that an attempt to write to the socket will block. For that it creates a server that will accept a websocket connection, but will not read anything from the socket input. The client side sends ping packets until the socket buffer is full - which is detected by setting up a 10s timeout and observing that the ping data could not be written during this time. The assumption of the test is that a write call that takes more than 10s is indicative that the buffers are full, and will never succeed. The problem occurs when the write succeeds after ~10s either because the kernel was busy doing some other things, or because the kernel suddenly decided to resize (increase) the buffers, which causes the write call to unblock and succeed after 10s. The test already had some provision and a workaround for that issue - via a repeatable( ) operation - but the workaround was only enabled for macOS where such behavior had first been observed. The fix extends that workaround for Windows - since the later failure shows that something similar is happening there. The fix also moves the websocket.abort() and following check inside the repeatable loop for better reliability. Since pushing test fixes during rampdown 2 is permitted, and since the failure was observed in the JDK 15 CI, I'm planning to push this test fix to the jdk15 repo, unless I hear any objection. best regards, -- daniel
[15] [testbug] 8249786: java/net/httpclient/websocket/PendingPingTextClose.java fails very infrequently
Hi, Please find below a fix for: 8249786: java/net/httpclient/websocket/PendingPingTextClose.java fails very infrequently https://bugs.openjdk.java.net/browse/JDK-8249786 webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8249786/webrev.00/ This test has been observed failing on windows in the JDK 15 CI. The most troublesome issue is that the test was producing so much output that the actual reason for the failure was lost in the output overflow. After instrumenting the test to limit the output and adding a few higher level traces, I was able to reproduce once (out of 250 runs) and see the actual stack trace. The test fails just after calling websocket.abort() when it tries to verify that the cfPing CompletableFuture completed exceptionally, and found that it actually successfully completed. The logic of the test is to try to fill up the local send buffer by sending ping messages, so that an attempt to write to the socket will block. For that it creates a server that will accept a websocket connection, but will not read anything from the socket input. The client side sends ping packets until the socket buffer is full - which is detected by setting up a 10s timeout and observing that the ping data could not be written during this time. The assumption of the test is that a write call that takes more than 10s is indicative that the buffers are full, and will never succeed. The problem occurs when the write succeeds after ~10s either because the kernel was busy doing some other things, or because the kernel suddenly decided to resize (increase) the buffers, which causes the write call to unblock and succeed after 10s. The test already had some provision and a workaround for that issue - via a repeatable( ) operation - but the workaround was only enabled for macOS where such behavior had first been observed. The fix extends that workaround for Windows - since the later failure shows that something similar is happening there. The fix also moves the websocket.abort() and following check inside the repeatable loop for better reliability. Since pushing test fixes during rampdown 2 is permitted, and since the failure was observed in the JDK 15 CI, I'm planning to push this test fix to the jdk15 repo, unless I hear any objection. best regards, -- daniel
Re: RFR 8245245 : WebSocket can loose the URL encoding of URI query parameters
> 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 LGTM. -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 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
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
> 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
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 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
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
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
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
Re: RFR 8244652: Add test for non utf-8 response handling by websocket client
Thanks Daniel
I would proceed with the push if there are no further comments.
On 13/05/2020, 12:39, "Daniel Fuchs" wrote:
Hi Rahul,
That looks good to me thanks!
I am still teetering on whether we should also have altered the
specification of WSHandshakeException, and explicitly changed the
signature of WSHandshakeException::getResponse to return an
HttpResponse instead of an HttpResponse.
On the one hand - our implementation will now actually instantiate
an HttpResponse. On the other hand - it might not be fair
to actually require it in the specification.
Maybe Pavel will have some thoughts on the subject.
As for me - your changes to the test look good.
best regards
-- daniel
On 12/05/2020 20:05, Rahul wrote:
> Hello,
>
> Request to have my fix reviewed for the issue:
>
>JDK-8244652: Add test for non utf-8 response handling by websocket
> client.
>
> The test java.net.httpclient.websocket.WSHandshakeExceptionTest.java is
> updated to test
>
>that the websocket client handles invalid utf-8 sent by the websocket
> server as part of the
>
>websocket exception whenthe handshake is rejected by websocket server.
>
>Issue: https://bugs.openjdk.java.net/browse/JDK-8244652
>
>Webrev:
> http://cr.openjdk.java.net/~ryadav/webrev_8244652/webrev.00/index.html
>
> - rahul
>
Re: RFR 8244652: Add test for non utf-8 response handling by websocket client
Hi Rahul, That looks good to me thanks! I am still teetering on whether we should also have altered the specification of WSHandshakeException, and explicitly changed the signature of WSHandshakeException::getResponse to return an HttpResponse instead of an HttpResponse. On the one hand - our implementation will now actually instantiate an HttpResponse. On the other hand - it might not be fair to actually require it in the specification. Maybe Pavel will have some thoughts on the subject. As for me - your changes to the test look good. best regards -- daniel On 12/05/2020 20:05, Rahul wrote: Hello, Request to have my fix reviewed for the issue: JDK-8244652: Add test for non utf-8 response handling by websocket client. The test java.net.httpclient.websocket.WSHandshakeExceptionTest.java is updated to test that the websocket client handles invalid utf-8 sent by the websocket server as part of the websocket exception whenthe handshake is rejected by websocket server. Issue: https://bugs.openjdk.java.net/browse/JDK-8244652 Webrev: http://cr.openjdk.java.net/~ryadav/webrev_8244652/webrev.00/index.html - rahul
RFR 8244652: Add test for non utf-8 response handling by websocket client
Hello, Request to have my fix reviewed for the issue: JDK-8244652: Add test for non utf-8 response handling by websocket client. The test java.net.httpclient.websocket.WSHandshakeExceptionTest.java is updated to test that the websocket client handles invalid utf-8 sent by the websocket server as part of the websocket exception when the handshake is rejected by websocket server. Issue: https://bugs.openjdk.java.net/browse/JDK-8244652 Webrev: http://cr.openjdk.java.net/~ryadav/webrev_8244652/webrev.00/index.html - rahul
Re: RFR 8240666: Websocket client’s OpeningHandshake discards the HTTP response body
On 06/05/2020 16:48, rahul.r.ya...@oracle.com wrote: Thanks Daniel , webrev updated. Looks good! -- daniel
Re: RFR 8240666: Websocket client’s OpeningHandshake discards the HTTP response body
Thanks Daniel , webrev updated.
- rahul
On 06/05/2020 16:16, Daniel Fuchs wrote:
Hi Rahul,
LGTM.
111 WebSocketHandshakeException wse =
(WebSocketHandshakeException) t;
112 out.println("Status code is " +
wse.getResponse().statusCode());
113 out.println("Response is " +
wse.getResponse().body());
114 assertNotNull(wse.getResponse());
I'd suggest moving line 114 two lines up (just after line 111)
to avoid triggering a NPE if wse.getResponse() returns null.
No need for a new webrev if that's the only change.
best regards,
-- daniel
On 06/05/2020 16:04, Rahul wrote:
Hi Pavel,
Thank you for the comment, the webrev has been updated.
webrev :
http://cr.openjdk.java.net/~ryadav/webrev_8240666/webrev.00/index.html
- rahul
On 06/05/2020, 14:26, "Pavel Rappo" wrote:
An assertion of the form
assertEquals(true,
((String)wse.getResponse().body()).contains("404"));
looks odd. I'd suggest using any of
assertTrue(boolean condition)
assertTrue(boolean condition, String message)
-Pavel
> On 6 May 2020, at 14:12, Rahul
wrote:
>
>
http://cr.openjdk.java.net/~ryadav/webrev_8240666/webrev.00/index.html
Re: RFR 8240666: Websocket client’s OpeningHandshake discards the HTTP response body
> On 6 May 2020, at 16:16, Daniel Fuchs wrote: > > Hi Rahul, > > LGTM. +1 -Chris.
Re: RFR 8240666: Websocket client’s OpeningHandshake discards the HTTP response body
Hi Rahul,
LGTM.
111 WebSocketHandshakeException wse =
(WebSocketHandshakeException) t;
112 out.println("Status code is " +
wse.getResponse().statusCode());
113 out.println("Response is " +
wse.getResponse().body());
114 assertNotNull(wse.getResponse());
I'd suggest moving line 114 two lines up (just after line 111)
to avoid triggering a NPE if wse.getResponse() returns null.
No need for a new webrev if that's the only change.
best regards,
-- daniel
On 06/05/2020 16:04, Rahul wrote:
Hi Pavel,
Thank you for the comment, the webrev has been updated.
webrev : http://cr.openjdk.java.net/~ryadav/webrev_8240666/webrev.00/index.html
- rahul
On 06/05/2020, 14:26, "Pavel Rappo" wrote:
An assertion of the form
assertEquals(true, ((String)wse.getResponse().body()).contains("404"));
looks odd. I'd suggest using any of
assertTrue(boolean condition)
assertTrue(boolean condition, String message)
-Pavel
> On 6 May 2020, at 14:12, Rahul wrote:
>
> http://cr.openjdk.java.net/~ryadav/webrev_8240666/webrev.00/index.html
Re: RFR 8240666: Websocket client’s OpeningHandshake discards the HTTP response body
Hi Pavel,
Thank you for the comment, the webrev has been updated.
webrev : http://cr.openjdk.java.net/~ryadav/webrev_8240666/webrev.00/index.html
- rahul
On 06/05/2020, 14:26, "Pavel Rappo" wrote:
An assertion of the form
assertEquals(true, ((String)wse.getResponse().body()).contains("404"));
looks odd. I'd suggest using any of
assertTrue(boolean condition)
assertTrue(boolean condition, String message)
-Pavel
> On 6 May 2020, at 14:12, Rahul wrote:
>
> http://cr.openjdk.java.net/~ryadav/webrev_8240666/webrev.00/index.html
Re: RFR 8240666: Websocket client’s OpeningHandshake discards the HTTP response body
An assertion of the form
assertEquals(true, ((String)wse.getResponse().body()).contains("404"));
looks odd. I'd suggest using any of
assertTrue(boolean condition)
assertTrue(boolean condition, String message)
-Pavel
> On 6 May 2020, at 14:12, Rahul wrote:
>
> http://cr.openjdk.java.net/~ryadav/webrev_8240666/webrev.00/index.html
RFR 8240666: Websocket client’s OpeningHandshake discards the HTTP response body
Hello, Request to have my fix reviewed for the issue: JDK-8240666: Websocket client’s OpeningHandshake discards the HTTP response body. The fix updates jdk.internal.net.http.websocket.OpeningHandshake.send() to process the response body received from server instead of discarding it, when the handshake is rejected by the websocket server. Issue: https://bugs.openjdk.java.net/browse/JDK-8240666 Webrev: http://cr.openjdk.java.net/~ryadav/webrev_8240666/webrev.00/index.html --rahul
Re: RFR: 8236859: WebSocket over authenticating proxy fails with NPE
Daniel, > 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/ There is a lot in this, but thanks to your very helpfully explanation, I think that it looks good. Having the extra wss test coverage is great. -Chris.
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: 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
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
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
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
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
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
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
RFR: 8236859: WebSocket over authenticating proxy fails with NPE
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[8217825]: Verify @AfterTest is used correctly in WebSocket tests
Thanks Patrick! I pushed it. best regards, -- daniel On 23/09/2019 18:13, Patrick Concannon wrote: Hi Pavel, Thanks for the feedback. I've incorporated the changes you suggested, and you can find them in the new webrev below. http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.02/ Kind regards, Patrick
Re: RFR[8217825]: Verify @AfterTest is used correctly in WebSocket tests
Hi Pavel,
Thanks for the feedback. I've incorporated the changes you suggested,
and you can find them in the new webrev below.
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.02/
Kind regards,
Patrick
On 20/09/2019 13:21, Pavel Rappo wrote:
Patrick,
Thank you for taking care of that!
Before you push, please add missing spaces to
WebSocketTest.java:612
WebSocketTest.java:713
(no need to update the webrev)
Nits:
WebSocketTest.java:808 does not abort WebSocket. Now, I understand that in
normal circumstances we never expect a WebSocket to be created. However, for
uniformity's sake, I would use abort here as well.
I understand that "single line" method formatting used in
AutomaticPong.java:140+ might not be everyone's cup of tea, but it might add to
readability in the case of a lengthy definition like that.
-Pavel
On 20 Sep 2019, at 11:36, Daniel Fuchs wrote:
Hi Patrick,
Looks good to me!
best regards,
-- daniel
On 20/09/2019 11:27, Patrick Concannon wrote:
Hi Daniel,
Thanks for the feedback. That's a good idea. I've made those changes and I've
included them in the webrev below.
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.01/
Kind regards,
Patrick
On 13/09/2019 15:47, Daniel Fuchs wrote:
Hi Patrick,
I wonder if you should be using `try { } finally { }` to ensure
that the websocket is closed too:
var websocket = .;
try {
// send some messages etc...
} finally {
websocket.abort();
}
best regards,
-- daniel
On 13/09/2019 15:36, Patrick Concannon wrote:
Hi,
Would it be possible to have my fix for JDK-8217825 - Verify @AfterTest is used
correctly in WebSocket tests - reviewed?
Following on from the discussion had here:
https://mail.openjdk.java.net/pipermail/net-dev/2019-January/012141.html, I’ve
refactored several websocket tests to explicitly close resources opened during
each test method i.e. test servers and websockets.
Tests refactored:
- test/jdk/java/net/httpclient/websocket/AutomaticPong.java
- test/jdk/java/net/httpclient/websocket/WebsocketTest.java
- test/jdk/java/net/httpclient/websocket/SendTest.java
- test/jdk/java/net/httpclient/websocket/Abort.java
Further information on this bug can be found here:
https://bugs.openjdk.java.net/browse/JDK-8217825
Webrev for fix:
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.00/
Kind regards,
Patrick
Re: RFR[8217825]: Verify @AfterTest is used correctly in WebSocket tests
Patrick,
Thank you for taking care of that!
Before you push, please add missing spaces to
WebSocketTest.java:612
WebSocketTest.java:713
(no need to update the webrev)
Nits:
WebSocketTest.java:808 does not abort WebSocket. Now, I understand that in
normal circumstances we never expect a WebSocket to be created. However, for
uniformity's sake, I would use abort here as well.
I understand that "single line" method formatting used in
AutomaticPong.java:140+ might not be everyone's cup of tea, but it might add to
readability in the case of a lengthy definition like that.
-Pavel
> On 20 Sep 2019, at 11:36, Daniel Fuchs wrote:
>
> Hi Patrick,
>
> Looks good to me!
>
> best regards,
>
> -- daniel
>
> On 20/09/2019 11:27, Patrick Concannon wrote:
>> Hi Daniel,
>> Thanks for the feedback. That's a good idea. I've made those changes and
>> I've included them in the webrev below.
>> http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.01/
>> Kind regards,
>> Patrick
>> On 13/09/2019 15:47, Daniel Fuchs wrote:
>>> Hi Patrick,
>>>
>>> I wonder if you should be using `try { } finally { }` to ensure
>>> that the websocket is closed too:
>>>
>>>var websocket = .;
>>>try {
>>> // send some messages etc...
>>>} finally {
>>> websocket.abort();
>>>}
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> On 13/09/2019 15:36, Patrick Concannon wrote:
>>>> Hi,
>
>
>>>>
>>>>
>>>> Would it be possible to have my fix for JDK-8217825 - Verify @AfterTest is
>>>> used correctly in WebSocket tests - reviewed?
>>>>
>>>> Following on from the discussion had here:
>>>> https://mail.openjdk.java.net/pipermail/net-dev/2019-January/012141.html,
>>>> I’ve refactored several websocket tests to explicitly close resources
>>>> opened during each test method i.e. test servers and websockets.
>>>>
>>>> Tests refactored:
>>>> - test/jdk/java/net/httpclient/websocket/AutomaticPong.java
>>>> - test/jdk/java/net/httpclient/websocket/WebsocketTest.java
>>>> - test/jdk/java/net/httpclient/websocket/SendTest.java
>>>> - test/jdk/java/net/httpclient/websocket/Abort.java
>>>>
>>>>
>>>> Further information on this bug can be found here:
>>>> https://bugs.openjdk.java.net/browse/JDK-8217825
>>>>
>
> Webrev for fix:
>>>> http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.00/
>>>>
>>>>
>
> Kind regards,
>
>>>>
>>>> Patrick
>>>>
>>>
>
Re: RFR[8217825]: Verify @AfterTest is used correctly in WebSocket tests
Hi Patrick,
Looks good to me!
best regards,
-- daniel
On 20/09/2019 11:27, Patrick Concannon wrote:
Hi Daniel,
Thanks for the feedback. That's a good idea. I've made those changes and
I've included them in the webrev below.
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.01/
Kind regards,
Patrick
On 13/09/2019 15:47, Daniel Fuchs wrote:
Hi Patrick,
I wonder if you should be using `try { } finally { }` to ensure
that the websocket is closed too:
var websocket = .;
try {
// send some messages etc...
} finally {
websocket.abort();
}
best regards,
-- daniel
On 13/09/2019 15:36, Patrick Concannon wrote:
Hi,
Would it be possible to have my fix for JDK-8217825 - Verify
@AfterTest is used correctly in WebSocket tests - reviewed?
Following on from the discussion had here:
https://mail.openjdk.java.net/pipermail/net-dev/2019-January/012141.html,
I’ve refactored several websocket tests to explicitly close resources
opened during each test method i.e. test servers and websockets.
Tests refactored:
- test/jdk/java/net/httpclient/websocket/AutomaticPong.java
- test/jdk/java/net/httpclient/websocket/WebsocketTest.java
- test/jdk/java/net/httpclient/websocket/SendTest.java
- test/jdk/java/net/httpclient/websocket/Abort.java
Further information on this bug can be found here:
https://bugs.openjdk.java.net/browse/JDK-8217825
Webrev for fix:
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.00/
Kind regards,
Patrick
Re: RFR[8217825]: Verify @AfterTest is used correctly in WebSocket tests
Hi Daniel,
Thanks for the feedback. That's a good idea. I've made those changes and
I've included them in the webrev below.
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.01/
Kind regards,
Patrick
On 13/09/2019 15:47, Daniel Fuchs wrote:
Hi Patrick,
I wonder if you should be using `try { } finally { }` to ensure
that the websocket is closed too:
var websocket = .;
try {
// send some messages etc...
} finally {
websocket.abort();
}
best regards,
-- daniel
On 13/09/2019 15:36, Patrick Concannon wrote:
Hi,
Would it be possible to have my fix for JDK-8217825 - Verify
@AfterTest is used correctly in WebSocket tests - reviewed?
Following on from the discussion had here:
https://mail.openjdk.java.net/pipermail/net-dev/2019-January/012141.html,
I’ve refactored several websocket tests to explicitly close resources
opened during each test method i.e. test servers and websockets.
Tests refactored:
- test/jdk/java/net/httpclient/websocket/AutomaticPong.java
- test/jdk/java/net/httpclient/websocket/WebsocketTest.java
- test/jdk/java/net/httpclient/websocket/SendTest.java
- test/jdk/java/net/httpclient/websocket/Abort.java
Further information on this bug can be found here:
https://bugs.openjdk.java.net/browse/JDK-8217825
Webrev for fix:
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.00/
Kind regards,
Patrick
Re: RFR[8217825]: Verify @AfterTest is used correctly in WebSocket tests
Hi Patrick,
I wonder if you should be using `try { } finally { }` to ensure
that the websocket is closed too:
var websocket = .;
try {
// send some messages etc...
} finally {
websocket.abort();
}
best regards,
-- daniel
On 13/09/2019 15:36, Patrick Concannon wrote:
Hi,
Would it be possible to have my fix for JDK-8217825 - Verify @AfterTest
is used correctly in WebSocket tests - reviewed?
Following on from the discussion had here:
https://mail.openjdk.java.net/pipermail/net-dev/2019-January/012141.html, I’ve
refactored several websocket tests to explicitly close resources opened
during each test method i.e. test servers and websockets.
Tests refactored:
- test/jdk/java/net/httpclient/websocket/AutomaticPong.java
- test/jdk/java/net/httpclient/websocket/WebsocketTest.java
- test/jdk/java/net/httpclient/websocket/SendTest.java
- test/jdk/java/net/httpclient/websocket/Abort.java
Further information on this bug can be found here:
https://bugs.openjdk.java.net/browse/JDK-8217825
Webrev for fix:
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.00/
Kind regards,
Patrick
RFR[8217825]: Verify @AfterTest is used correctly in WebSocket tests
Hi, Would it be possible to have my fix for JDK-8217825 - Verify @AfterTest is used correctly in WebSocket tests - reviewed? Following on from the discussion had here: https://mail.openjdk.java.net/pipermail/net-dev/2019-January/012141.html, I’ve refactored several websocket tests to explicitly close resources opened during each test method i.e. test servers and websockets. Tests refactored: - test/jdk/java/net/httpclient/websocket/AutomaticPong.java - test/jdk/java/net/httpclient/websocket/WebsocketTest.java - test/jdk/java/net/httpclient/websocket/SendTest.java - test/jdk/java/net/httpclient/websocket/Abort.java Further information on this bug can be found here: https://bugs.openjdk.java.net/browse/JDK-8217825 Webrev for fix: http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.00/ Kind regards, Patrick
Re: RFR [13] 8217976: test/jdk/java/net/httpclient/websocket/WebSocketProxyTest.java fails intermittently
Looks good. While you're at it you could make: 123 CopyOnWriteArrayList connections; final. No need for a new webrev. best regards, -- daniel On 29/01/2019 12:53, Chris Hegarty wrote: Agreed. Should be good enough for test cleanup. http://cr.openjdk.java.net/~chegar/8217976/webrev.01/ -Chris.
Re: RFR [13] 8217976: test/jdk/java/net/httpclient/websocket/WebSocketProxyTest.java fails intermittently
> On 29 Jan 2019, at 12:31, Daniel Fuchs wrote:
>
> ...
> The best would be to use CopyOnWriteArrayList instead
> of synchronizedList(). Otherwise you'd still need to copy the list
> within a synchronize(connections) { } block to prevent CME.
Agreed. Should be good enough for test cleanup.
http://cr.openjdk.java.net/~chegar/8217976/webrev.01/
-Chris.
Re: RFR [13] 8217976: test/jdk/java/net/httpclient/websocket/WebSocketProxyTest.java fails intermittently
Hi Chris,
116 List localList = List.copyOf(connections);
I think it is still possible for CME to happen during the copyOf
operation, even though it can reduce the window in which that might
happen.
The best would be to use CopyOnWriteArrayList instead
of synchronizedList(). Otherwise you'd still need to copy the list
within a synchronize(connections) { } block to prevent CME.
best regards,
-- daniel
On 29/01/2019 12:15, Chris Hegarty wrote:
WebSocketProxyTest is a new test that was added recently. It fails once
in every few hundred runs. The test uses a preexisting test-only proxy
server. There is a race when closing the server; the close method
iterates over all connections while another thread may be adding a new
connection. The solution proposed here is to take a copy of the
connection list and iterate over it, rather than the connection list
itself. ( I also did a little clean up and added some consistency to
proxy debug messages, since it was more difficult to reason about the
test's behavior from its log than it should have been )
http://cr.openjdk.java.net/~chegar/8217976/webrev.00/
-Chris.
RFR [13] 8217976: test/jdk/java/net/httpclient/websocket/WebSocketProxyTest.java fails intermittently
WebSocketProxyTest is a new test that was added recently. It fails once in every few hundred runs. The test uses a preexisting test-only proxy server. There is a race when closing the server; the close method iterates over all connections while another thread may be adding a new connection. The solution proposed here is to take a copy of the connection list and iterate over it, rather than the connection list itself. ( I also did a little clean up and added some consistency to proxy debug messages, since it was more difficult to reason about the test's behavior from its log than it should have been ) http://cr.openjdk.java.net/~chegar/8217976/webrev.00/ -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
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
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
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
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
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
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: WebSocket
> On 20 Feb 2018, at 23:49, Simone Bordet wrote: > > I don't see why the message should be reassembled, given > MessagePart.[FIRST|PART|LAST] ? > It's not that the user can ask for the kind of MessagePart, e.g. ask > only for WHOLE messages that the implementation must reassemble on her > behalf, no ? > So it will *always* be the case that zero copy is possible, right ? Simone, this was said in the context of guaranteed receiving of WHOLE messages. Initially we were talking about a possibility of providing buffering (i.e. assembling/accumulating) in the implementation [1][2]. For partial messages zero-copy is the case though [3]. I believe we have already decided we are not providing buffering in JDK 11. Your example has clearly shown how conceptually simple it is to consume whole messages on the user side, should the user need it. And unless the API changes to hand a ByteBuffer[] instead of a single ByteBuffer, the user can process whole messages more effectively (arguably, as there is still some overhead because of creating instances of CompletionStage). -Pavel [1] http://mail.openjdk.java.net/pipermail/net-dev/2018-February/011156.html [2] http://mail.openjdk.java.net/pipermail/net-dev/2018-February/011171.html [3] Just to keep it mind, it is only true until there are no extensions in the pre-defined maximum message size in the equation (RFC 6455): Unless specified otherwise by an extension, frames have no semantic meaning. An intermediary might coalesce and/or split frames, if no extensions were negotiated by the client and the server or if some extensions were negotiated, but the intermediary understood all the extensions negotiated and knows how to coalesce and/or split frames in the presence of these extensions.
Re: WebSocket
> On 20 Feb 2018, at 20:49, Simone Bordet wrote: > > Hi, > > On Tue, Feb 20, 2018 at 9:35 PM, Chris Hegarty > wrote: >> Optimistically, if the whole message is read from the underlying >> socket in one native read ( small messages ), then the whole >> message will be in a single byte buffer, zero copy. > > Good. > >> If not, then to re-assemble the whole message into a single byte buffer >> will require some copying ( there is no composite byte buffer). > > I don't see why the message should be reassembled, given > MessagePart.[FIRST|PART|LAST] ? > It's not that the user can ask for the kind of MessagePart, e.g. ask > only for WHOLE messages that the implementation must reassemble on her > behalf, no ? > So it will *always* be the case that zero copy is possible, right ? Correct. I though that the conversation was related to the Java EE API where the API supports requesting WHOLE messages, which cannot be guaranteed to be zero-copy. The Java SE API does not support requesting WHOLE messages so does not have this issue. -Chris.
Re: WebSocket
Hi, On Tue, Feb 20, 2018 at 9:35 PM, Chris Hegarty wrote: > Optimistically, if the whole message is read from the underlying > socket in one native read ( small messages ), then the whole > message will be in a single byte buffer, zero copy. Good. > If not, then to re-assemble the whole message into a single byte buffer > will require some copying ( there is no composite byte buffer). I don't see why the message should be reassembled, given MessagePart.[FIRST|PART|LAST] ? It's not that the user can ask for the kind of MessagePart, e.g. ask only for WHOLE messages that the implementation must reassemble on her behalf, no ? So it will *always* be the case that zero copy is possible, right ? -- Simone Bordet --- 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
Re: WebSocket
Simone, > On 20 Feb 2018, at 20:19, Simone Bordet wrote: > > Hi, > > On Tue, Feb 20, 2018 at 8:35 PM, Pavel Rappo wrote: >> If by copies you mean allocating new buffers and feeling them with the same >> contents, then you are right. No copies are made. > > I mean that you call user code with a ByteBuffer produced from a > slice() of an internal ByteBuffer. That should be the case. > The CF mechanism must ensure that the content of the internal > ByteBuffer is not changed (for example by clearing it and reading from > the network) before all CFs associated with sliced ByteBuffers > generated from the internal ByteBuffer are completed. > >> However there is a natural need for multiple passes over the buffers, both >> being sent and received. These are unavoidable transformations performed by >> SSL >> and WebSocket masking. > > Sure. > >> As for the WHOLE message, there's no way an implementation can provide a >> zero-copy >> solution. > > Leaving SSL aside, I don't see why not ? Optimistically, if the whole message is read from the underlying socket in one native read ( small messages ), then the whole message will be in a single byte buffer, zero copy. If not, then to re-assemble the whole message into a single byte buffer will require some copying ( there is no composite byte buffer). The whole message in the Java EE API is represented by a single byte buffer. The Java SE API does not suffer this issue. -Chris.
Re: WebSocket
Hi, On Tue, Feb 20, 2018 at 8:35 PM, Pavel Rappo wrote: > If by copies you mean allocating new buffers and feeling them with the same > contents, then you are right. No copies are made. I mean that you call user code with a ByteBuffer produced from a slice() of an internal ByteBuffer. The CF mechanism must ensure that the content of the internal ByteBuffer is not changed (for example by clearing it and reading from the network) before all CFs associated with sliced ByteBuffers generated from the internal ByteBuffer are completed. > However there is a natural need for multiple passes over the buffers, both > being sent and received. These are unavoidable transformations performed by > SSL > and WebSocket masking. Sure. > As for the WHOLE message, there's no way an implementation can provide a > zero-copy > solution. Leaving SSL aside, I don't see why not ? Read from network, parse header bytes, slice the buffer, call the listener. Zero copy. Can you expand ? -- Simone Bordet --- 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
Re: WebSocket
Thanks Simone, I think this is a good example of how this low-level API can be leveraged to build adapters, InputStream in this case. Equally, that could be a RS Subscriber, or some other non-blocking async JSON parser, or something else. Let’s not get caught up in what the JDK should provide out-of-the-box and what is likely to be provided by the community. However, what we must be right, in this release, is the lowest level foundational API, other things can come later or be provided by others in the community. -Chris. > On 20 Feb 2018, at 18:31, Simone Bordet wrote: > > Hi, > > On Tue, Feb 20, 2018 at 4:32 PM, Chuck Davis wrote: >> Simone, please, please tell me how this is done. > > The WebSocket.Listener APIs return a CompletableFuture, indicating > when the application is done with the processing of the parameters > passed to the listener method (in particular the ByteBuffer). > We can leverage this to avoid to copy the bytes in the ByteBuffer - we > will just store the ByteBuffers in the ByteBufferInputStream for later > use, and we return a CompletableFuture that we will complete when we > have consumed the ByteBuffer bytes. > > When the listener tells you that you have the LAST (or WHOLE) part, > then you can wrap the ByteBufferInputStream with your > ObjectInputStream. > > The code is attached and shows the bare minimum implementation of > ByteBufferInputStream - just implementing `int read()`. > Implementing `int read(byte[], int, int)` is left as exercise :) > > Just to be paranoid, the attached code license is public domain as > explained in https://creativecommons.org/publicdomain/zero/1.0/. > > Just to be clear, the only copy that happens is when you have to > bridge from the byte[] format used by InputStream to the ByteBuffer > format. > The same copy happens when you use ByteArrayInputStream (that you used > in the "good" code in your first email of this thread), so this > version is on par with your "good" code. > > This version has the advantage that guarantees that no other copy is > done, not even by the implementation (hopefully, right Pavel ?), while > with JDK 8 there may have been hidden copies to provide a full > ByteBuffer. > > -- > Simone Bordet > --- > 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 >
Re: WebSocket
> On 20 Feb 2018, at 21:31, Simone Bordet wrote: > > > > This version has the advantage that guarantees that no other copy is > done, not even by the implementation (hopefully, right Pavel ?), while > with JDK 8 there may have been hidden copies to provide a full > ByteBuffer. > If by copies you mean allocating new buffers and feeling them with the same contents, then you are right. No copies are made. However there is a natural need for multiple passes over the buffers, both being sent and received. These are unavoidable transformations performed by SSL and WebSocket masking. As for the WHOLE message, there's no way an implementation can provide a zero-copy solution. Thanks, -Pavel
Re: WebSocket
Thank you so much for your assistance, Simone. On Tue, Feb 20, 2018 at 10:31 AM, Simone Bordet wrote: > Hi, > > On Tue, Feb 20, 2018 at 4:32 PM, Chuck Davis wrote: > > Simone, please, please tell me how this is done. > > > This version has the advantage that guarantees that no other copy is > done, not even by the implementation (hopefully, right Pavel ?), while > with JDK 8 there may have been hidden copies to provide a full > ByteBuffer. > > - >
Re: WebSocket
Hi,
On Tue, Feb 20, 2018 at 4:32 PM, Chuck Davis wrote:
> Simone, please, please tell me how this is done.
The WebSocket.Listener APIs return a CompletableFuture, indicating
when the application is done with the processing of the parameters
passed to the listener method (in particular the ByteBuffer).
We can leverage this to avoid to copy the bytes in the ByteBuffer - we
will just store the ByteBuffers in the ByteBufferInputStream for later
use, and we return a CompletableFuture that we will complete when we
have consumed the ByteBuffer bytes.
When the listener tells you that you have the LAST (or WHOLE) part,
then you can wrap the ByteBufferInputStream with your
ObjectInputStream.
The code is attached and shows the bare minimum implementation of
ByteBufferInputStream - just implementing `int read()`.
Implementing `int read(byte[], int, int)` is left as exercise :)
Just to be paranoid, the attached code license is public domain as
explained in https://creativecommons.org/publicdomain/zero/1.0/.
Just to be clear, the only copy that happens is when you have to
bridge from the byte[] format used by InputStream to the ByteBuffer
format.
The same copy happens when you use ByteArrayInputStream (that you used
in the "good" code in your first email of this thread), so this
version is on par with your "good" code.
This version has the advantage that guarantees that no other copy is
done, not even by the implementation (hopefully, right Pavel ?), while
with JDK 8 there may have been hidden copies to provide a full
ByteBuffer.
--
Simone Bordet
---
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
/**
* Licensed under public domain
* https://creativecommons.org/publicdomain/zero/1.0/
*/
import java.io.InputStream;
import java.io.ObjectInputStream;
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;
import jdk.incubator.http.WebSocket;
public class WebSocketTest
{
private static class ByteBufferInputStream extends InputStream
{
private final List buffers = new ArrayList<>();
private final List> callbacks = new ArrayList<>();
private int index;
public CompletableFuture offer(ByteBuffer byteBuffer)
{
buffers.add(byteBuffer);
CompletableFuture result = new CompletableFuture<>();
callbacks.add(result);
return result;
}
@Override
public int read()
{
while (index < buffers.size())
{
ByteBuffer byteBuffer = buffers.get(index);
if (byteBuffer.hasRemaining())
return byteBuffer.get() & 0xFF;
callbacks.get(index).complete(null);
++index;
}
return -1;
}
}
private static class MyListener implements WebSocket.Listener
{
private final ByteBufferInputStream bbis = new ByteBufferInputStream();
@Override
public CompletionStage onBinary(WebSocket webSocket, ByteBuffer message, WebSocket.MessagePart part)
{
CompletableFuture result = bbis.offer(message);
if (part == WebSocket.MessagePart.LAST || part == WebSocket.MessagePart.WHOLE)
{
ObjectInputStream ois = new ObjectInputStream(bbis);
System.err.println("ois.readObject() = " + ois.readObject());
}
return result;
}
}
}
Re: WebSocket
> ByteBufferInputStream bbis = new ByteBufferInputStream();
> for each partial ByteBuffer received by the listener {
> bbis.add(byteBuffer);
> }
> ois = new ObjectInputStream(bbis);
> MyDTO = ois.readObject();
>
> Now, ByteBufferInputStream is not present in the JDK, and if you want
> to complain you are in good (and conspicuous) company, as the JDK
> engineers appear to avoid the issue since years now (e.g. create a
> String from a ByteBuffer without copy).
> Having said that, the class is trivial to implement (a naive version I
> wrote is about 30 lines) and may even be provided as a utility class
> by the JDK WebSocket implementers.
>
> The advantage is that ByteBufferInputStream can be written in a way
> that performs *zero* copies, thanks to the JDK 9 WebSocket APIs -
> add() would need to return a CompletableFuture.
> If you don't want to confront with CompletableFutures, it can be
> written in a way that performs just one copy.
>
Simone, please, please tell me how this is done. I'm too dense to see it.
I really REALLY do not like my vision of reassembling all the bits and
pieces so that deserialization can occur. It's critically important that I
learn the BEST way to get thousands of rows from my database to my client.
At this point I'm contemplating whether I'd be better off giving up the
bidirectional advantage of WebSocket and going back to RMI. If I
absolutely needed server push I could implement a JMS but that is a huge
increase of complexity. How do I reassemble everything with *zero* or only
1 copy?
Thanks and I will forever be your debtor!!!
Re: WebSocket
The conversation has gone a little off topic from the WebSocket API, and this is probably not the best place to be discussing such proposed convenience / performance APIs, since they need broader discussion than net-dev. It seems most folk are in agreement that the proposed WebSocket API could be used as a foundation to build the desired higher-level abstractions that have been asked for on this thread. I see this as a win for the proposed low-level Java SE WebSocket API. It is supposed to be a building block, not a complete building. -Chris. > On 20 Feb 2018, at 10:52, Simone Bordet wrote: > > Hi, > > On Tue, Feb 20, 2018 at 10:22 AM, Alan Bateman > wrote: >> >> >> On 20/02/2018 08:14, Simone Bordet wrote: >>> >>> : >>> Where would be a good list to start discussing ByteBuffer to >>> [Input|Output]Stream bridging ? >>> >> Are you looking for this for performance or convenience reasons? > > Convenience. > >> Bridging channel and input/output streams is natural of course, bridging >> between >> buffers and input/output streams hasn't come up very often. ByteBuffer >> defines bulk get/put methods so it's trivial for anyone to wrap byte buffers >> if they need to. > > Sure, it just would be handy the JDK provided this rather than users > rewriting the same utility class. > > For example, having: > > OutputStream.write(ByteBuffer) > InputStream.read(ByteBuffer) > > avoiding intermediate copies to byte[] but also: > > ByteBufferInputStream.offer(ByteBuffer b, boolean last) > > to feed ByteBuffers to the InputStream and be able to read them via > normal byte[]. > > Something along these lines. > > Thanks ! > > -- > Simone Bordet > --- > 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
Re: WebSocket
Hi, On Tue, Feb 20, 2018 at 10:22 AM, Alan Bateman wrote: > > > On 20/02/2018 08:14, Simone Bordet wrote: >> >> : >> Where would be a good list to start discussing ByteBuffer to >> [Input|Output]Stream bridging ? >> > Are you looking for this for performance or convenience reasons? Convenience. > Bridging channel and input/output streams is natural of course, bridging > between > buffers and input/output streams hasn't come up very often. ByteBuffer > defines bulk get/put methods so it's trivial for anyone to wrap byte buffers > if they need to. Sure, it just would be handy the JDK provided this rather than users rewriting the same utility class. For example, having: OutputStream.write(ByteBuffer) InputStream.read(ByteBuffer) avoiding intermediate copies to byte[] but also: ByteBufferInputStream.offer(ByteBuffer b, boolean last) to feed ByteBuffers to the InputStream and be able to read them via normal byte[]. Something along these lines. Thanks ! -- Simone Bordet --- 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
Re: WebSocket
On 20/02/2018 08:14, Simone Bordet wrote: : Where would be a good list to start discussing ByteBuffer to [Input|Output]Stream bridging ? Are you looking for this for performance or convenience reasons? Bridging channel and input/output streams is natural of course, bridging between buffers and input/output streams hasn't come up very often. ByteBuffer defines bulk get/put methods so it's trivial for anyone to wrap byte buffers if they need to. -Alan
Re: WebSocket
Alan, On Tue, Feb 20, 2018 at 8:50 AM, Alan Bateman wrote: > On 19/02/2018 20:19, Simone Bordet wrote: >> >> : >> Now, ByteBufferInputStream is not present in the JDK, and if you want >> to complain you are in good (and conspicuous) company, as the JDK >> engineers appear to avoid the issue since years now (e.g. create a >> String from a ByteBuffer without copy). >> > There is an active proposal/discussion on core-libs-dev to add a constructor > to String that creates the String by decoding the bytes in a buffer. Thanks for this ! Where would be a good list to start discussing ByteBuffer to [Input|Output]Stream bridging ? -- Simone Bordet --- 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
Re: WebSocket
On 19/02/2018 20:19, Simone Bordet wrote: : Now, ByteBufferInputStream is not present in the JDK, and if you want to complain you are in good (and conspicuous) company, as the JDK engineers appear to avoid the issue since years now (e.g. create a String from a ByteBuffer without copy). There is an active proposal/discussion on core-libs-dev to add a constructor to String that creates the String by decoding the bytes in a buffer. -Alan
Re: WebSocket
Hi, On Mon, Feb 19, 2018 at 10:55 PM, Chuck Davis wrote: >> Note also that your requirement is to use blocking, stream-based, >> byte[]-based APIs. >> If you had chosen a data format for which a non-blocking parser based >> on ByteBuffer APIs existed, you would be so happy about the JDK 9 >> APIs. > > Like what for instance? I'm looking for suggestions/strategies how best to > adapt to the future. It's very unlikely I'm going to convert all my data to > text before sending across the wire. Starting point: https://en.wikipedia.org/wiki/Comparison_of_data_serialization_formats CBOR looks promising and it's a standard. If you don't need references, then many other choices. I don't know the status of the libraries around CBOR though, so yes, I understand the appeal of Java Serialization. As I said, a JDK bridge between ByteBuffer and [Input|Output]Stream will solve a million cases. -- Simone Bordet --- 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
Re: WebSocket
Hi Simone:
Thanks for jumping in with your thoughts.
On Mon, Feb 19, 2018 at 12:19 PM, Simone Bordet
wrote:
> Hi,
>
> Well, not so fast.
> There are a lot of assumptions in the above code.
> The first is that the ByteBuffer has a backing array, which may not be the
> case.
> If you write the right code that takes care of the possibility that
> the ByteBuffer does not have a backing array, you end up with another
> copy and more code.
> More importantly, the implementation would have done all the copies
> for you in order to feed your code with a final ByteBuffer.
> So you are not really skipping the copies and the performance is not
> different from below, where the possible copies are explicit.
>
Fortunately the Oracle JVM implements a backing array and the code I put
out is exactly from my implementation and has been working superbly for the
last year. Obviously, the day is approaching when I have to run on openJDK
and I don't know if there is a backing array there since it is optional per
the docs.
> > DONE! More complicated, messy and very slow. (i.e. lots of wasted cpu
> cycles)
>
> Nah :)
>
> ByteBufferInputStream bbis = new ByteBufferInputStream();
> for each partial ByteBuffer received by the listener {
> bbis.add(byteBuffer);
> }
> ois = new ObjectInputStream(bbis);
> MyDTO = ois.readObject();
>
> Now, ByteBufferInputStream is not present in the JDK, and if you want
> to complain you are in good (and conspicuous) company, as the JDK
> engineers appear to avoid the issue since years now (e.g. create a
> String from a ByteBuffer without copy).
> Having said that, the class is trivial to implement (a naive version I
> wrote is about 30 lines) and may even be provided as a utility class
> by the JDK WebSocket implementers.
>
> The advantage is that ByteBufferInputStream can be written in a way
> that performs *zero* copies, thanks to the JDK 9 WebSocket APIs -
> add() would need to return a CompletableFuture.
> If you don't want to confront with CompletableFutures, it can be
> written in a way that performs just one copy.
>
Now we're talking strategy.go man!!
>
> To sum up:
> * with JDK 9 you need a utility class that you did not need with JDK 8
> * with JDK 9 you can actually perform a zero copy provided you put a
> bit of effort (or the JDK guys do)
> * with JDK 9 the number of byte copies is the same or better
>
> All in all, anything you can do with the JDK 8 API can be done in JDK 9.
> For some cases, JDK 9 is more efficient than JDK 8.
> For all other cases it's on par.
> JDK 9 requires a utility class to convert from ByteBuffer to InputStream.
>
You can do it but being the lazy creature I am I want it done for menow
they're going to make me do it myself..
>
> Note also that your requirement is to use blocking, stream-based,
> byte[]-based APIs.
> If you had chosen a data format for which a non-blocking parser based
> on ByteBuffer APIs existed, you would be so happy about the JDK 9
> APIs.
>
Like what for instance? I'm looking for suggestions/strategies how best to
adapt to the future. It's very unlikely I'm going to convert all my data
to text before sending across the wire.
>
> I have a different experience, where clients may not be able to keep
> up with the network.
> This happens with browsers receiving a message and having to update a
> gazillion DOM nodes (with all the ripple effects that this entails),
> with clients generating so much garbage that the GC is heavily
> interfering with their throughput, with proxy applications that have
> different speeds on the 2 sides they proxy, and so on.
>
> You have my sincerest sympathy. I feel sorry for anybody who
has to interface with that pathetic pile of garbage we refer to as a
browser and it's even more pathetic protocol. You'd think our
industry could do better.
> Here I am seriously advocating to the net-dev JDK group to provide an
> efficient solution, or at least start a discussion where appropriate,
> for ByteBuffer to [Input|Output]Stream that is sorely missing in the
> JDK.
>
> Thanks !
>
Can we start a "Me too" movement? 😊
P. S. If you don't know about our silly American "Me too" movement
consider yourself lucky!
>
>
Re: WebSocket
Hi,
On Sun, Feb 18, 2018 at 9:12 PM, Chuck Davis wrote:
> There is a great hush here regarding strategy for putting the pieces
> back together so here goes my tirade.
>
> Following is based on assumption of a 100k payload.
>
> With jdk8:
> The decoder received a ByteBuffer
> bais = new ByteArrayInputStream(ByteBuffer.array()); // wrap the
> buffer -- optional method but implemented in Oracle JVM
> ois = new ObjectInputStream(bais) // wrap the input stream
> MyDTO = ois.readObject(); // deserialize the object (which may
> contain hundreds or thousands of objects)
>
> DONE! Simple, clean and blazingly fast.
Well, not so fast.
There are a lot of assumptions in the above code.
The first is that the ByteBuffer has a backing array, which may not be the case.
If you write the right code that takes care of the possibility that
the ByteBuffer does not have a backing array, you end up with another
copy and more code.
More importantly, the implementation would have done all the copies
for you in order to feed your code with a final ByteBuffer.
So you are not really skipping the copies and the performance is not
different from below, where the possible copies are explicit.
> With jdk9:
> The Listener receives a ByteBuffer (I read someplace the default size
> is potentially going to be 256 bytes)
> If it's the first part (or whole) create a byte[] of 10k (or whatever)
> Start copying bytes from the buffer to the array
> Check each byte to be sure there is room in the array for another byte
> if room, copy the byte
> if no room, copy the array to a larger array (current size + 10k)
> and then copy the byte
> repeat until all messages are processed (i.e. each byte will have
> been copied somewhere between 1 and 10 times)
> bais = new ByteArrayInputStrea(byte[]);
> ois = new ObjectInputStream(bais);
> MyDTO = ois.readObject();
>
> DONE! More complicated, messy and very slow. (i.e. lots of wasted cpu
> cycles)
Nah :)
ByteBufferInputStream bbis = new ByteBufferInputStream();
for each partial ByteBuffer received by the listener {
bbis.add(byteBuffer);
}
ois = new ObjectInputStream(bbis);
MyDTO = ois.readObject();
Now, ByteBufferInputStream is not present in the JDK, and if you want
to complain you are in good (and conspicuous) company, as the JDK
engineers appear to avoid the issue since years now (e.g. create a
String from a ByteBuffer without copy).
Having said that, the class is trivial to implement (a naive version I
wrote is about 30 lines) and may even be provided as a utility class
by the JDK WebSocket implementers.
The advantage is that ByteBufferInputStream can be written in a way
that performs *zero* copies, thanks to the JDK 9 WebSocket APIs -
add() would need to return a CompletableFuture.
If you don't want to confront with CompletableFutures, it can be
written in a way that performs just one copy.
> RESULT: The formerly fabulous WebSocket has been rendered relatively
> useless as a platform for building responsive, bidirectional
> client/server applications. Am I the only person who sees this as a
> HUGE regression of functionality?? I am ALARMED by this turn of
> events.
Nah :)
To sum up:
* with JDK 9 you need a utility class that you did not need with JDK 8
* with JDK 9 you can actually perform a zero copy provided you put a
bit of effort (or the JDK guys do)
* with JDK 9 the number of byte copies is the same or better
All in all, anything you can do with the JDK 8 API can be done in JDK 9.
For some cases, JDK 9 is more efficient than JDK 8.
For all other cases it's on par.
JDK 9 requires a utility class to convert from ByteBuffer to InputStream.
Note also that your requirement is to use blocking, stream-based,
byte[]-based APIs.
If you had chosen a data format for which a non-blocking parser based
on ByteBuffer APIs existed, you would be so happy about the JDK 9
APIs.
> OPINION:
>
> I'm pretty naive about the world of midrange and mainframe except that
> they can throw a lot of bytes in a short time. But I imagine the
> choke-point of any desktop application is going to be the network.
> Unless somebody is running a Pentium I doubt any relatively modern
> desktop is going to have a difficult time keeping up with networking
> speeds. It seems to me, therefore, that backpressure in WebSocket is
> a solution looking for a problem.
I have a different experience, where clients may not be able to keep
up with the network.
This happens with browsers receiving a message and having to update a
gazillion DOM nodes (with all the ripple effects that this entails),
with clients generating so much garbage that the GC is heavily
interfering with their throughput, with proxy applications that have
different speeds on the 2 sides they proxy, and so on.
Here I am seriously advocating to the net-dev JDK group to provide
Re: WebSocket
Further thought... Maybe a single WebSocket but two different listeners the developer can choose depending upon what type of stream is expected???
Re: WebSocket
Hi Chris and others: There is only one WebSocket documented in jdk9. I was happy to be able to write a jdk9 client that connected to my server unchanged (using Wildfly11). And there are features of jdk9 API that I rather like. There are two vastly differing handling requirements. I certainly am not smart enough to figure out how to handle both in the same implementation. With all credit to James the need to send Java objects over a wire is not going to go away anytime soon. But deserialization cannot commence until the entire payload generated by the ObjectOutputStream is in a single buffer at the destination (whether client or server). Let's call this the buffer processing approach. Then you have the crowd who need to process a stream of music, video or you name it. In that case you want the jdk9 approach to process smaller bits of the stream as they arrive. Let's call this the stream processing approach. While both approaches are streams the streams are so vastly different I don't see any way to process them both with a single API implementation. But as I said, I am not the sharpest knife in the drawer and maybe you guys have the smarts to pull it off. I certainly hope so. The current jdk9 implementation is not suitable for data-driven applications that have to process a buffer. Putting all the pieces back together is prohibitively expensive in time and resources. For those who need to process a stream the jdk9 implementation appears to be a great leap forward. I maintain we need both. On Mon, Feb 19, 2018 at 3:45 AM, Chris Hegarty wrote: > > On 18/02/18 20:12, Chuck Davis wrote: > >> ... >> >> RESULT: The formerly fabulous WebSocket has been rendered relatively >> useless as a platform for building responsive, bidirectional >> client/server applications. Am I the only person who sees this as a >> HUGE regression of functionality?? I am ALARMED by this turn of >> events. >> > > A point of clarification that may, or may not, be obvious > to readers of this thread. > > The WebSocket API, with JDK 8, that is being compared is the > Java EE API for WebSocket [1]. It is completely orthogonal to > the proposed Java SE WebSocket API. Users of said API can > continue to use it, if it meets their needs. Nothing being > proposed here has any impact on it. > > The Java EE API for WebSocket provides higher-level API > abstractions for handling whole and partial messages, decoding > and encoding, etc. The proposed Java SE WebSocket API is a > less-verbose, lower-level, API. They are two separate and > distinct APIs. If one desires the the higher-level abstractions > provided by the EE API, that is absolutely fine. > > The SE WebSocket API is not a replacement for the EE API, it > is a lower-level alternative with simpler abstractions. A > consequence of its simplicity is that a developer may need > to do more to handle the data, but in many cases this is > exactly what it wanted. These APIs complement each other > rather than compete. > > -Chris > > [1] https://docs.oracle.com/javaee/7/api/javax/websocket/package > -summary.html >
Re: WebSocket
On 18/02/18 20:12, Chuck Davis wrote: ... RESULT: The formerly fabulous WebSocket has been rendered relatively useless as a platform for building responsive, bidirectional client/server applications. Am I the only person who sees this as a HUGE regression of functionality?? I am ALARMED by this turn of events. A point of clarification that may, or may not, be obvious to readers of this thread. The WebSocket API, with JDK 8, that is being compared is the Java EE API for WebSocket [1]. It is completely orthogonal to the proposed Java SE WebSocket API. Users of said API can continue to use it, if it meets their needs. Nothing being proposed here has any impact on it. The Java EE API for WebSocket provides higher-level API abstractions for handling whole and partial messages, decoding and encoding, etc. The proposed Java SE WebSocket API is a less-verbose, lower-level, API. They are two separate and distinct APIs. If one desires the the higher-level abstractions provided by the EE API, that is absolutely fine. The SE WebSocket API is not a replacement for the EE API, it is a lower-level alternative with simpler abstractions. A consequence of its simplicity is that a developer may need to do more to handle the data, but in many cases this is exactly what it wanted. These APIs complement each other rather than compete. -Chris [1] https://docs.oracle.com/javaee/7/api/javax/websocket/package-summary.html
Re: WebSocket
James, who taught you how to ruin other people's days? You seem to have
mastered the art..😐
There seems to be a move afoot to abandon serialization.hummm. Then it
needs to get fixed because converting everything to a text stream before
transmitting is not an option (for me at least). Then it needs to be
converted back to an object to play nice with Java when it reaches the
client. I just did a quick look and there are no JSON classes in jdk9. I
hope I never have to stoop so low as to use the HTTP protocol. If so, I
want it to be under the covers where I never have to see it.
I'm going to continue to pursue my course because I have a fairly secure
environment. I'll never bring in anything through the firewall. All my
serialization is done in an ejb container with my specific versions of
commonly defined classes on both server and client and only one class can
ever be deserialized (although it is a container for other specific
classes). If something else was going to be injected it would have to be
between the ejb container and my user's JVM which in my naivety seems quite
unlikely. If the result does not conform to my class definition things
should blow up.
That said, I do need to study some on reactive streams since it seems to be
the future.
Thanks for the info and the links to more info.
On Sun, Feb 18, 2018 at 2:42 PM, James Roper wrote:
> Hi Chuck,
>
> Let's just imagine that for a moment that there existed a reactive streams
> equivalent to an ObjectReader (there isn't, and there's good reason why
> there isn't, but I'll get to that later). Then, the code would be
something
> like this (the Source API there is an Akka Streams like API):
>
> HttpRequest.create(URI.create("..."))
> .GET()
> .response((status, headers) ->
> BodySubscriber.from(
> Source.asPublisher()
> Now, as I said, no reactive streams implementation (that I'm aware of)
> provides Java serialization support, and for good reason. It's 2018, Java
> serialization has been shown, time and time again, to have major security
> flaws in it. If you make an HTTP request on another application, and parse
> what it gives back to you using ObjectInputStream, you are opening a quite
> trivial way for that application to execute code on your application. Even
> if you trust the remote system, it goes against the security best practice
> of bulkheading - ensuring that if one application is compromised, the
entire
> system isn't compromised. No one wants to provide support these days for
> such insecure practices.
>
> Here's a good summary of why you should never use Java serialization:
>
> https://www.christian-schneider.net/JavaDeserializationSecurityFAQ.html
>
> Regards,
>
> James
>
Re: WebSocket
Hi Chuck,
Let's just imagine that for a moment that there existed a reactive streams
equivalent to an ObjectReader (there isn't, and there's good reason why
there isn't, but I'll get to that later). Then, the code would be something
like this (the Source API there is an Akka Streams like API):
HttpRequest.create(URI.create("..."))
.GET()
.response((status, headers) ->
BodySubscriber.from(
Source.asPublisher()
.via(ReactiveStreamsObjectReader.create())
.forEach(object -> {
// do what you want with each object as its passed here
})
)
);
Now that actually looks like less, and simpler, code than your
implementation - the above is the full code for the example, you only
included code for handling the body. You can also do a lot more than that,
you declaratively define how you process your objects, create complex
graphs feeding them to other locations, etc. I'd suggest you read the docs
on Reactive Streams implementations, because one of the goals of the JDK9
client is to be asynchronous, and being asynchronous means you do have to
turn your processing on its heaḋ, you don't pull (ie, you don't invoke
readObject and get something back), rather, you are pushed things, so you
define stages in your processing, and provide callbacks when you want to do
custom things. Here's the docs for Akka streams to get acquainted with an
asynchronous streaming view of the world:
https://doc.akka.io/docs/akka/2.5/stream/index.html
Now, as I said, no reactive streams implementation (that I'm aware of)
provides Java serialization support, and for good reason. It's 2018, Java
serialization has been shown, time and time again, to have major security
flaws in it. If you make an HTTP request on another application, and parse
what it gives back to you using ObjectInputStream, you are opening a quite
trivial way for that application to execute code on your application. Even
if you trust the remote system, it goes against the security best practice
of bulkheading - ensuring that if one application is compromised, the
entire system isn't compromised. No one wants to provide support these days
for such insecure practices.
Here's a good summary of why you should never use Java serialization:
https://www.christian-schneider.net/JavaDeserializationSecurityFAQ.html
Regards,
James
On 18 February 2018 at 20:12, Chuck Davis wrote:
> There is a great hush here regarding strategy for putting the pieces
> back together so here goes my tirade.
>
> Following is based on assumption of a 100k payload.
>
> With jdk8:
> The decoder received a ByteBuffer
> bais = new ByteArrayInputStream(ByteBuffer.array()); // wrap the
> buffer -- optional method but implemented in Oracle JVM
> ois = new ObjectInputStream(bais) // wrap the input stream
> MyDTO = ois.readObject(); // deserialize the object (which may
> contain hundreds or thousands of objects)
>
> DONE! Simple, clean and blazingly fast.
>
> With jdk9:
> The Listener receives a ByteBuffer (I read someplace the default size
> is potentially going to be 256 bytes)
> If it's the first part (or whole) create a byte[] of 10k (or whatever)
> Start copying bytes from the buffer to the array
> Check each byte to be sure there is room in the array for another byte
> if room, copy the byte
> if no room, copy the array to a larger array (current size + 10k)
> and then copy the byte
> repeat until all messages are processed (i.e. each byte will have
> been copied somewhere between 1 and 10 times)
> bais = new ByteArrayInputStrea(byte[]);
> ois = new ObjectInputStream(bais);
> MyDTO = ois.readObject();
>
> DONE! More complicated, messy and very slow. (i.e. lots of wasted cpu
> cycles)
>
> RESULT: The formerly fabulous WebSocket has been rendered relatively
> useless as a platform for building responsive, bidirectional
> client/server applications. Am I the only person who sees this as a
> HUGE regression of functionality?? I am ALARMED by this turn of
> events.
>
> OPINION:
>
> I'm pretty naive about the world of midrange and mainframe except that
> they can throw a lot of bytes in a short time. But I imagine the
> choke-point of any desktop application is going to be the network.
> Unless somebody is running a Pentium I doubt any relatively modern
> desktop is going to have a difficult time keeping up with networking
> speeds. It seems to me, therefore, that backpressure in WebSocket is
> a solution looking for a problem. If backpressure is somehow
> essential in WebSocket include the possibility but please don't
> destroy the best client/server communication in the process. If
> necessary, create two implementations: WebSocketFast and
> WebSocketSlow.
>
> I'll go back to my cave now and pout about what has happened to my
> once fabulous WebSocket.
>
--
*James Roper*
*Senior Octonaut*
Lightbend <https://www.lightbend.com/> – Build reactive apps!
Twitter: @jroper <https://twitter.com/jroper>
