GH-3980: Fix DefaultSftpSFactory for concurrency #3981
Conversation
Fixes spring-projects#3980 When not `isSharedSession`, the `initClient()` is called for every session we request from the factory and in concurrent calls we end up with not initialized SSH client in some threads. * Add `synchronized` to the `initClient()` to block other threads while the first one initialize the client
|
I prepared slightly different approach with Double-checked locking: #3982 My approach might be slightly faster because:
|
| } | ||
|
|
||
| private void initClient() throws IOException { | ||
| private synchronized void initClient() throws IOException { |
There was a problem hiding this comment.
Synchronization has to be higher up - otherwise if isSharedSession is true, we could orphan a session; see code in getSession.
There was a problem hiding this comment.
If isSharedSession we do this.sharedSessionLock.lock();
|
|
||
| private void initClient() throws IOException { | ||
| private synchronized void initClient() throws IOException { | ||
| if (this.initialized.compareAndSet(false, true)) { |
There was a problem hiding this comment.
this.initialized should not be set at the beginning of the method.
If initialisation fails, the variable will be set to true for uninitialized client.
Also, we can use volatile boolean instead of AtomicBoolean
There was a problem hiding this comment.
Yeah... Makes sense.
OK closing my PR.
Feel free to copy my unit test into your PR then we will review it and merge properly.
|
Closed in favor: #3982 |
3 participants
Fixes #3980
When not
isSharedSession, theinitClient()is called for every session we request from the factory and in concurrent calls we end up with not initialized SSH client in some threads.synchronizedto theinitClient()to block other threads while the first one initialize the client