Fix custom SSLSocketFactory not being set because of an unsafe lazy-initialization in JDK

[ahaasler]

Hi,

this PR fixes #3293 with an approach similar to the one contributed by @kevinconaway on jersey/jersey#3738 (check up his well written explanation). We (@amplia-iiot) think his contribution was forgotten when the project was transitioned (#3738) over to the Eclipse Foundation and have implemented this more maintainable approach (the change is easier to notice when looking at secureConnection(JerseyClient, HttpURLConnection) and avoids multiple calls to HttpsURLConnection.getDefaultSSLSocketFactory() making it cleaner).

I've not included the original test by @kevinconaway because I'm not aware of his status regarding the Eclipse Contributor Agreement, but it can be checked at @6a8e80ec625f2ce9a7585dc45e645fab61aa504c (bugfix/jersey#3293-test).

We think this is the way to go for Jersey until the JDK properly makes this lazy-initialization thread-safe like the implementation in Android's JDK.

Nevertheless, we are open to feedback regarding this change and discussing alternate approaches.

[jansupol]

The test nicely tests what is fixed by this PR and I think it would be good to include it. Given that the original PR of @kevinconaway contained the code, and it could have been contributed to the Eclipse Foundation along with other Jersey code, I think it is safe to include it.

[kevinconaway]

Please let me know if you need me to sign a CLA or something for the original code.

[ahaasler]

I've added you with the same name and email as your original commit:

Add test for concurrent custom SSLSocketFactory

Test made by Kevin Conaway @kevinconaway

Also-by: Kevin Conaway <[email protected]>
Signed-off-by: Adrian Haasler García <[email protected]>

I don't know if the Also-by section causes the eclipsefdn/eca check to fail if it has not been signed. Let me know if you sign the agreement with a different email so I can change the commit message.

[jansupol]

@kevinconaway If you can sign the ECA, it would be great. Please follow the steps here.

[ahaasler]

How should I proceed with this contribution?

[jansupol]

@kevinconaway We would be happy if you help us here. If you cannot, please let us know. Thank you.

[kevinconaway]

I have signed the ECA

[ahaasler]

Thanks @kevinconaway, did you sign it with the same email that appears on the commit message amplia-iiot@6a8e80e or should I update it?

[ahaasler]

Some tests have failed:

For JDK11 -Ptravis_e2e

[ERROR]   ClientExecutorTest.testDefaultExecutorRx:88 
Expected: a string containing "jersey-client-async-executor"
     but: was ""

For JDK15 -Ptravis_e2e

[ERROR]   BroadcasterTest.test:188 Waiting for resultsA1 to be complete failed.

I doubt they have anything to do with the change in the first commit, how may I help?

[kevinconaway]

Thanks @kevinconaway, did you sign it with the same email that appears on the commit message amplia-iiot/jersey@6a8e80e or should I update it?

You may update it to the email address that was used to sign the ECA

[jbescos]

Are we sure this fixes something?. I don't agree with what is written here: https://github.com/jersey/jersey/issues/3293

HttpsURLConnection.getDefaultSSLSocketFactory() can be called simultaneously by multiple threads, returning a different object for each invocation...

It is true that HttpsURLConnection.getDefaultSSLSocketFactory() is not thread safe, but it doesn't really matter because it relies in SSLSocketFactory.getDefault(). This one is thread safe and it will always return the same instance.

So it is possible that 1+ threads invokes HttpsURLConnection.getDefaultSSLSocketFactory() when defaultSSLSocketFactory is null, and then it is set more than 1 time. But it doesn't matter because the instance is the same.

[ahaasler]

This is the piece of code in the SDK that contains the lazy-initialization unsafe code:

public static SSLSocketFactory getDefaultSSLSocketFactory() {
    if (defaultSSLSocketFactory == null) {
        defaultSSLSocketFactory =
            (SSLSocketFactory)SSLSocketFactory.getDefault();
    }
    return defaultSSLSocketFactory;
}

It doesn't matter that SSLSocketFactory.getDefault() is thread-safe because getDefaultSSLSocketFactory may return to the second thread an object that is not null but may not have been constructed yet. More info on the matter:

• http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
• https://www.oracle.com/technical-resources/articles/javase/bloch-effective-08-qa.html

This causes the jersey comparison to fail and the custom SSLSocketFactory is not set.

We have seen this in production and this patch solves it.

[jbescos]

I see, 'theFactory' is not always set in the SSLSocketFactory.getDefault(). Thanks.

Initially I saw that SSLSocketFactory.getDefault() returns the already instanced 'theFactory':

/*     */   public static synchronized SocketFactory getDefault()
/*     */   {
/*  89 */     if (theFactory != null) {
/*  90 */       return theFactory;
/*     */     }

But sometimes 'theFactory' is not instanced.