okhttp: support JDK9 ALPN#4136
Conversation
|
|
||
| @Test | ||
| public void negotiate_handshakeFails() throws IOException { | ||
| SSLParameters parameters = mock(SSLParameters.class); |
There was a problem hiding this comment.
Why not new SSLParameters()? Please use real objects when easily possible.
| SSLParameters.class.getMethod("setApplicationProtocols", String[].class); | ||
| Method getApplicationProtocol = | ||
| AccessController.doPrivileged( | ||
| new PrivilegedExceptionAction<Method>() { |
There was a problem hiding this comment.
Why is this privileged and not the earlier getMethod?
There was a problem hiding this comment.
An oversight. Fixed.
| return (String) getApplicationProtocol.invoke(socket); | ||
| } catch (IllegalAccessException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (InvocationTargetException e) { |
There was a problem hiding this comment.
It is known that getApplicationProtocol can throw UnsupportedOperationException. This case should be handled more cleanly.
For one, we can do e.getCause(). From there we can either propagate it directly (if it is a RuntimeException) or wrap it and provide a message with additional information.
(Actually, if we verify the provider supports this method ahead-of-time, then I'm fine with this current code.)
There was a problem hiding this comment.
Now verify at startup that an SSLEngine created using the provider does not throw when getApplicationProtocol is invoked.
| return SSLSocket.class.getMethod("getApplicationProtocol"); | ||
| } | ||
| }); | ||
| return new JdkAlpnPlatform(sslProvider, setApplicationProtocols, getApplicationProtocol); |
There was a problem hiding this comment.
We should verify that sslProvider supports ALPN by calling getApplicationProtocol().
There was a problem hiding this comment.
Done. AFAICT this requires instantiating an SSLEngine to invoke getApplicationProtocol(), which is the approach now taken.
2 participants
Adds support for JDK9's built-in ALPN to the OkHttp transport.
Manually tested with JDK9. I removed the alpnagent from the interop-testing dependencies to test this locally, but did not remove them in this PR (I'm not sure if our test infrastructure is all running on JDK9 yet).
The first commit contains two test changes that are necessary to build on JDK9 (one copied from #3638).