Optimize DefaultSftpSessionFactory

[artembilan]

Related to https://build.spring.io/browse/INT-MASTERSPRING40-677/

Doesn't look like DefaultSftpSessionFactory.getSession() needs
locking around sharedJschSession

• change the logic in the getSession() to store a sharedJschSession
  into the local variable and if it is null or not connected, create a
  new JSchSessionWrapper, connect it and store to the sharedJschSession
  back into the sharedJschSession property if this.isSharedSession.
  This way we always deal with sharedJschSession anyway if it is valid
  or create a new fresh one if that is invalid.
  Without locking we always get an actual state of the sharedJschSession
  and don't fall into the race condition when sharedJschSession is invalid,
  but we can't connect to the SFTP channel from the sftpSession.connect()

[artembilan]

I won't insist for this change, but read/write locking brings us a race condition when we fail in the sftpSession.connect() out of writeLock.

At least that is how I understand it...

[garyrussell]

But this won't work if two threads call getSession()` concurrently; each will get a different session and the last one wins.

I haven't looked at the locking code lately, but why not just replace it with a simple synchronized(this)?

[artembilan]

Hm. Yes, I see what you mean: more concurrency more JSchSessionWrapper instances are going to be spawned.
Does it really matter for us or the point is really try to keep only a single instance ?
Then we really always need to make access to this sharedJschSession as blocked...

We might just then have a a regular lock around it...

I'll think more.

Thank you!

[artembilan]

How about this?

[garyrussell]

Why write it back to the field on every call?

In any case, it must be done while the lock is held; otherwise we're back to the possibility of a concurrent update.