On Wed, 16 Jun 2021 09:07:49 GMT, Daniel Fuchs <dfu...@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.
>
> 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...)
Good idea on the CF or latch. As regards the blind cast, I suppose I could test
for it and if the type is different, leave the executor reference as null,
which reverts current behavior, but I can't imagine a scenario where that would
actually happen. If it did, would we want to revert the behavior, or fail
visibly with CCE?
-------------
PR: https://git.openjdk.java.net/jdk/pull/4506
