Issue #10547 - Allow Executor of WebSocketClient to be customized via HttpClient

[joakime]

Add ability of WebSocketClient to pull Executor and ByteBufferPool from configured HttpClient

[lorban]

LGTM but please note that the issue was originally reported for Jetty 10.0.x.

Don't you want to do this change on the jetty-10.0.x branch then merge it forward?

[joakime]

LGTM but please note that the issue was originally reported for Jetty 10.0.x.

Don't you want to do this change on the jetty-10.0.x branch then merge it forward?

This is the jetty-10.0.x branch.
What are you seeing?

[lorban]

I could swear I saw this PR made against 12.0.x. Either I dreamt it or GitHub was drunk.

[lachlan-roberts]

LGTM, just an issue with the the javadoc.

[gregw]

The change is good... but I don't like some of pre-existing stuff I see.
I'm not keen on how there is both a ThreadPool and an Executor in the WebSocketComponents.

Ideally we should just pass one and derive the other from it (up or down caste). If they really REALLY can be different, then it needs to be javadoc'd what each is used for and how they relate.

So, whilst only tangential to this PR, can you add some javadoc about that?

[joakime]

The change is good... but I don't like some of pre-existing stuff I see.

Sounds like we need a new Issue about it.

I'm not keen on how there is both a ThreadPool and an Executor in the WebSocketComponents.

There's only an Executor in WebSocketComponents.
The QueuedThreadPool you see is the default implementation if an Executor is null.

Ideally we should just pass one and derive the other from it (up or down caste). If they really REALLY can be different, then it needs to be javadoc'd what each is used for and how they relate.

There's an Executor in WebSocketComponents, and an Executor in HttpClient.
For purposes of WebSocket / WebSocketClient the WebSocketComponents executor is used.
For purposes of HTTP, the HttpClient executor one is used.
If you give WebSocketClient an unstarted and/or unconfigured HttpClient, then the Executor on the HttpClient is populated from the WebSocketComponents.

So, whilst only tangential to this PR, can you add some javadoc about that?

That seems complicated and is documenting code / internal behaviors, I don't see that as appropriate for an API doc (once documented, we cannot change the internal behaviors).

[gregw]

That seems complicated and is documenting code / internal behaviors, I don't see that as appropriate for an API doc (once documented, we cannot change the internal behaviors).

I don't understand how to use this class, so the javadoc is for people like me.
@lachlan-roberts can you say how the usage of the threadpool vs the executor differ?

[joakime]

@lachlan-roberts can you say how the usage of the threadpool vs the executor differ?

@gregw where do you see threadpool and executor in the same class?

[lachlan-roberts]

@gregw I think we never needed to use any methods of ThreadPool in websocket except execute(). So WebSocketComponents just provides an Executor instead of a ThreadPool.

In the ServerWebSocketComponents we use the servers ThreadPool for this.