On Wed, 16 Jun 2021 07:56:11 GMT, Michael McMahon <micha...@openjdk.org> wrote:
> Hi,
>
> This fixes a problem where the listener methods of a WebSocket client were
> being invoked by the Selector manager thread, which is problematic, because
> if the implementation of any of these methods tries to do any blocking work,
> this impacts other http activity, and if the blocking work is a http client
> call, then a hang can result. The fix makes the HttpClient's executor
> available to WebSocketImpl and that is used to offload the listener
> invocations.
>
> The fix also adds a more comprehensive test framework for WebSockets (in
> WebSocketServer). Up to now we just had a limited server side in the tests
> that can only do the handshake. This change adds an API and implementation
> for server's to receive websocket messages and send replies back to clients.
> To implement this, the server hooks into WebSocket's Frame, MessageEncoder
> and MessageDecoder classes. Therefore, the implementations of these classes
> had to be modified to allow for the encoding of server generated messages and
> the decoding of client generated messages. The only difference between client
> and server in this respect relates to payload masking which is only done for
> messages sent from client to server.
>
> There's a couple of warts that I wasn't sure what to do with. 1) There is
> already a copy of the Frame implementation class in the test hierarchy. I
> presume this is used by other tests, but that implementation is not used by
> this change. 2) The WebSocketServer is based on the existing
> DummyWebSocketServer class which is used by other tests. I can't see any easy
> way to refactor/combine these implementations.
>
> Thanks,
>
> Michael.
Great fix and great test Michael.
I don't specially like the blind cast to HttpClientFacade - maybe we can
revisit that later.
I have only one comment - about the use of Thread.sleep(3_000) in the test.
test/jdk/java/net/httpclient/websocket/java.net.http/jdk/internal/net/http/websocket/WebSocketAndHttpClient.java
line 38:
> 36: HttpTest httpTest = new HttpTest(httpClient, args[1]);
> 37:
> 38: AtomicReference<String> result = new AtomicReference<>("failed");
Would it be possible to use a CountDownLatch - or a CompletableFuture<String>
instead of an atomic reference in order to replace the Thread.sleep(3_000) at
line 44 with latch.await() or cf.join()?
The test would then fail in jtreg timeout without the fix, but would not have
to wait for an arbitrary magic 3s delay if the fix is present (which might not
be enough delay on slow machines with debug builds etc...)
-------------
PR: https://git.openjdk.java.net/jdk/pull/4506
