RFR: 8263743: redundant lock in SSLSocketImpl

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8263743: redundant lock in SSLSocketImpl

Xue-Lei Andrew Fan
Remove redundant lock in SSLSocketImpl.

In the SSLSocketImpl, there is a socket level lock while reading application data (see readApplicationRecord).

                socketLock.lock();
                try {
                    plainText = decode(buffer);
                } finally {
                    socketLock.unlock();
                }
If an application data read is in progress, other calling to SSLSocket APIs (for example getUseClientMode()) could be blocked if socket level locks are used.

No new regression test.  Simple fix, hard to trigger the deadlock.

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

Commit messages:
 - 8263743: redundant lock in SSLSocketImpl

Changes: https://git.openjdk.java.net/jdk/pull/3053/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3053&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263743
  Stats: 7 lines in 1 file changed: 0 ins; 6 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3053.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3053/head:pull/3053

PR: https://git.openjdk.java.net/jdk/pull/3053
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263743: redundant lock in SSLSocketImpl

Xue-Lei Andrew Fan
On Wed, 17 Mar 2021 17:23:03 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

> Remove redundant lock in SSLSocketImpl.
>
> In the SSLSocketImpl, there is a socket level lock while reading application data (see readApplicationRecord).
>
>                 socketLock.lock();
>                 try {
>                     plainText = decode(buffer);
>                 } finally {
>                     socketLock.unlock();
>                 }
> If an application data read is in progress, other calling to SSLSocket APIs (for example getUseClientMode() in a handshake complete listener) could be blocked if socket level locks are used.
>
> No new regression test.  Simple fix, hard to trigger the deadlock.

ping ...

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

PR: https://git.openjdk.java.net/jdk/pull/3053
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263743: redundant lock in SSLSocketImpl

Jamil Nimeh-2
In reply to this post by Xue-Lei Andrew Fan
On Wed, 17 Mar 2021 17:23:03 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

> Remove redundant lock in SSLSocketImpl.
>
> In the SSLSocketImpl, there is a socket level lock while reading application data (see readApplicationRecord).
>
>                 socketLock.lock();
>                 try {
>                     plainText = decode(buffer);
>                 } finally {
>                     socketLock.unlock();
>                 }
> If an application data read is in progress, other calling to SSLSocket APIs (for example getUseClientMode() in a handshake complete listener) could be blocked if socket level locks are used.
>
> No new regression test.  Simple fix, hard to trigger the deadlock.

After reading over the description in the bug I think this looks fine.

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

Marked as reviewed by jnimeh (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3053
Reply | Threaded
Open this post in threaded view
|

Integrated: 8263743: redundant lock in SSLSocketImpl

Xue-Lei Andrew Fan
In reply to this post by Xue-Lei Andrew Fan
On Wed, 17 Mar 2021 17:23:03 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

> Remove redundant lock in SSLSocketImpl.
>
> In the SSLSocketImpl, there is a socket level lock while reading application data (see readApplicationRecord).
>
>                 socketLock.lock();
>                 try {
>                     plainText = decode(buffer);
>                 } finally {
>                     socketLock.unlock();
>                 }
> If an application data read is in progress, other calling to SSLSocket APIs (for example getUseClientMode() in a handshake complete listener) could be blocked if socket level locks are used.
>
> No new regression test.  Simple fix, hard to trigger the deadlock.

This pull request has now been integrated.

Changeset: a678a38d
Author:    Xue-Lei Andrew Fan <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/a678a38d
Stats:     7 lines in 1 file changed: 0 ins; 6 del; 1 mod

8263743: redundant lock in SSLSocketImpl

Reviewed-by: jnimeh

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

PR: https://git.openjdk.java.net/jdk/pull/3053