fix #1170: Use ReentrantLock instead of Object monitors

[carterkozak]

This is not a complete implementation of #1170, but does address the primary areas where I've encountered virtual thread pinning in (non-exhaustive) tests. I realize this draft PR is premature as we haven't had a chance to discuss options on the issue yet -- I'd put this much together for some testing elsewhere, and thought I'd share it. No hard feelings if you'd prefer a different approach :-)

I suspect we may want to handle ConscryptFileDescriptorSocket in addition to the default ConscryptEngineSocket to avoid surprises, but I can certainly imagine an argument against updating the old (deprecated?) implementation.

[prbprbprb]

Thanks for this!

I think you can actually go a bit further - ConscryptEngine.sslLock and ConscryptEngineSocket.stateLock can be ReadWriteLocks and the common code paths only need the read lock. handshakeLock needs to be a mutex though.

[carterkozak]

I think ReadWriteLocks may be the right way to go here, but I'm not confident that I have enough context on what we're guarding to make the right call. For example ConscryptEngine.sslLock seems to guard the native ssl object (as described in the locks javadoc) but also the state field. NativeSsl itself uses a ReadWriteLock internally, so I suspect at some point in the past the NativeSsl type was extracted from something like ConscryptEngine.
ReadWriteLock, even reentrant variants, require us to be careful about recursive calls because upgrading from a read lock to a write lock is dangerous. While I've read through the implementation, I'm not yet confident that I've internalized the code enough to guarantee upgrades are not possible.
As you describe in #1170 (comment), access to SSLEngine/SSLSocket tends to be single threaded, so I wouldn't expect to gain much by introducing a ReadWriteLock over a mutex. Changing the read-path locking semantics could expose thread safety issues in calling code (Concretely the current mutex on ConscryptEngineSocket.SSLInputStream may protect us from some concurrency issues -- I don't think this in isolation is reason not to make the change if it doesn't match locking in the JDKs implementation, but I'd be hesitant to change it at the same time as moving away from object monitors).

Given this, I'd like to move forward initially with ReentrantLock because it should produce a semantically equivalent behavior, and should introduce less risk because the change is primarily structural. Moving from ReentrantLock to ReentrantReadWriteLock doesn't necessarily require structural changes, and could be handled later as an iterative step.

Let me know if this sounds reasonable to you, thanks!

[prbprbprb]

Changing the read-path locking semantics could expose thread safety issues in calling code

Makes sense... I remember now that I started looking into tidying up the state transitions in ConscryptEngine along the lines of #1120 and the realised it's a bigger project that I thought. So yeah, let's take things one step at a time.

[prbprbprb]

Looks like the ReadWrite locking in #490 and was done in isolation to address finalizer races (i.e. when NativeSSl.ssl itself becomes invalid), without really considering calling classes or non-close related methods.

ConscryptEngine.sslLock seems to guard the native ssl object (as described in the locks javadoc) but also the state field

It's worse than that, it also seems to be used as a general purpose mutex around ConscryptEngine internals.... There should probably be 3 different locks, but not this CL because that's asking for trouble.

Better yet, references to ConscryptEngine.ssl can escape as part of ConscryptEngine.activeSession, which also synchronizes on it. So I think you'll need to push the RentrantLock down into NativeSsl itself.

Which is annoying because that means you'll have to keep ConscryptFileDescriptorSocket in this CL... I was going to suggest dropping it as it's beyond deprecated but we still have one major user, who are very change-averse.

[prbprbprb]

provideAfterHandshakeSession() also needs locking (my bad!)

[carterkozak]

Happy to fix it up. We may be able to add a compileOnly("com.google.errorprone:error_prone_annotations:$errorproneVersion") to take advantage of @GuardedBy checks without introducing a dependency for consumers.

[carterkozak]

The exportKeyingMaterial is interesting because it locks over the state check, but not the NativeSsl interaction:

conscrypt/common/src/main/java/org/conscrypt/ConscryptEngine.java

Lines 1839 to 1849 in b14310d

	byte[] exportKeyingMaterial(String label, byte[] context, int length) throws SSLException {
	sslLock.lock();
	try {
	if (state < STATE_HANDSHAKE_COMPLETED || state == STATE_CLOSED) {
	return null;
	}
	} finally {
	sslLock.unlock();
	}
	return ssl.exportKeyingMaterial(label, context, length);
	}

[carterkozak]

Better yet, references to ConscryptEngine.ssl can escape as part of ConscryptEngine.activeSession, which also synchronizes on it. So I think you'll need to push the RentrantLock down into NativeSsl itself.

Ah, great catch, thanks! I'll fix this up, and take another pass over other code which synchronizes on NativeSsl.