RFR: 8246039: SSLSocket HandshakeCompletedListeners are run on virtual threads

[Bradford Wetmore]

On Wed, 16 Sep 2020 13:28:05 GMT, Carter Kozak <github.com+3854321+carterkozak at openjdk.org> wrote:

>> Thanks for identifying this potential use-case.
>> 
>> I found the discussion from Jan 2013 on security-dev about this issue but I was unable to find a conclusion or the
>> rational for why the listeners weren't called directly (which would of course would stall finishHashshake). So I think
>> we should re-start that discussion and get a conclusion on whether this will be changed or not. Minimally we need
>> javax.net.ssl.SSLSocket clarified to set expectations on the execution context and whether it runs asynchronously from
>> the finish (the session may be invalidated before or while the listener runs).  Assuming security-dev wants to continue
>> to create thread per handshake notification then your patch make sense, just need to drop inheritThreadLocals (the
>> final parameter Thread constructor in TransportContext is false, meaning that inheritable thread locals are not
>> inherited in the existing code).
>
>> I think we should re-start that discussion and get a conclusion on whether this will be changed or not.
> 
> I completely agree that running listeners on the completion thread would be ideal and allow the most flexibility for
> consumers, but I'm worried that changing the behavior between java releases without a new API (perhaps an overload
> which additionally takes an Executor instance) would cause too much friction for existing consumers. Listeners which
> expect to execute long running operations would potentially cause performance regressions when taking a java release
> including the change, which I'd imagine would make it a non-starter. Loom virtual threads provide the ability to
> preserve existing behavior without the overhead incurred by OS threads. I'd be happy to start a thread to discuss a
> mechanism to listen for handshake completion without dispatching to additional threads (virtual or otherwise), but I
> think that would be a separate, parallel, change. What do you think?
>> just need to drop inheritThreadLocals (the final parameter Thread constructor in TransportContext is false, meaning
>> that inheritable thread locals are not inherited in the existing code)
> 
> Good catch, I got my wires crossed! Fixed.

>From a coding point of view, if Xuelei doesn't have a further suggestion, using virtual threads like you have suggested
seems to be a good solution.  I'm ok with this proposed change for Project Loom.  Loom has a lot of promise for issues
like this.  Looking through the Thread builder source, it looks like right options are captured.

However, let's give Xuelei a bit of time to think of other options before integration.

-------------

PR: https://git.openjdk.java.net/loom/pull/16