Add locking around read/write in NativeSsl. #490
Conversation
This uses a read/write lock around the ssl instance variable for NativeSsl. The write lock is only taken during close(), where ssl is cleared, so all other operations can proceed in parallel with one another. I only added locking to the read- and write-style methods in the class, rather than to methods that only read or write a property, since the latter tend to be used only right when the SSL is created and it would add a lot of noise to the code to lock everywhere, but it's possible we want to add that as well for complete safety. This should solve some longstanding but infrequent crashes we've seen that involve race conditions with finalizers and other related situations. Fixes google#455.
|
The benchmark numbers here are suspiciously good. Specifically, they indicate a net positive effect on performance when I ran them (and I ran them again because I didn't believe it the first time, with a similar result, this is the second set). I assume that's probably a side effect of something on my machine happening to execute at the same time as the reference benchmark but not with the new code, but it should indicate that the cost of this isn't particularly high. |
| try { | ||
| if (isClosed() || fd == null || !fd.valid()) { | ||
| throw new SocketException("Socket is closed"); | ||
| } |
There was a problem hiding this comment.
This if () block is repeated in several places. Why not extract it out into a separate function ? (maybe with an "@GuardedBy" ?
There was a problem hiding this comment.
That seems like a good idea for a future change, yeah.
| return NativeCrypto.ENGINE_SSL_do_handshake(ssl, this, handshakeCallbacks); | ||
| lock.readLock().lock(); | ||
| try { | ||
| return NativeCrypto.ENGINE_SSL_do_handshake(ssl, this, handshakeCallbacks); |
There was a problem hiding this comment.
Why do some methods have the isClosed check whereas others do not ? (other examples, readDirectByteBuffer, writeDirectByteBuffer and forceRead)
There was a problem hiding this comment.
The pattern is that the methods used by SSLSocket have the check and the ones used by SSLEngine don't, but they should probably all check for being closed. (Note that it's a case of which exception is thrown rather than correctness, as the native code checks for ssl == 0 and throws NullPointerException in that case. It'd be better to throw SSLException if it's closed.) Another change I'd like to push to a future change.
|
@narayank As a general comment, I'd like to keep this as small as possible so it can be cherry-picked with minimal conflicts and risk, so I'd rather put refactorings into another change. |
|
Alright, I reran the benchmarks on a GCE instance, which should hopefully be more reproducible, and got more sensible results with an average slowdown of 0.50%. I think that's a completely acceptable cost for safety from race conditions, so I'm going to commit this. |
This uses a read/write lock around the ssl instance variable for
NativeSsl. The write lock is only taken during close(), where ssl is
cleared, so all other operations can proceed in parallel with one
another. I only added locking to the read- and write-style methods in
the class, rather than to methods that only read or write a property,
since the latter tend to be used only right when the SSL is created
and it would add a lot of noise to the code to lock everywhere, but
it's possible we want to add that as well for complete safety.
This should solve some longstanding but infrequent crashes we've seen
that involve race conditions with finalizers and other related
situations.
Fixes #455.