okhttp: add socketFactory method to channel builder #5378
Conversation
| @@ -156,6 +158,14 @@ public final OkHttpChannelBuilder transportExecutor(@Nullable Executor transport | |||
| return this; | |||
| } | |||
|
|
|||
| /** | |||
| * Override the default {@link SocketFactory} used to create sockets. | |||
There was a problem hiding this comment.
Might be better to doc the behavior of null input.
| @@ -234,6 +245,7 @@ protected void handleNotInUse() { | |||
| // Client initiated streams are odd, server initiated ones are even. Server should not need to | |||
| // use it. We start clients at 3 to avoid conflicting with HTTP negotiation. | |||
| nextStreamId = 3; | |||
| this.socketFactory = socketFactory; | |||
There was a problem hiding this comment.
Can the field be final?
this.socketFactory = socketFactory == null ? SocketFactory.getDefault() : socketFactory;
There was a problem hiding this comment.
The javadoc says:
Factory classes are specified by environment-specific configuration mechanisms. For example, the getDefault method could return a factory that was appropriate for a particular user or applet, and a framework could use a factory customized to its own purposes.
While I believe that in general the return value of SocketFactory.getDefault() is hard-coded and cannot change at runtime, this doesn't seem to be guaranteed by the API - so I went with the safer (?) approach of invoking it whenever used, which should mimic the existing behavior of new Socket(...)
There was a problem hiding this comment.
That field can still be final. You might also need to test SocketFactory.getDefault() is used.
I think every single okhttp transport creates for only once a socket almost immediately after the transport is instantiated. It does not make much sense that the transport should be responsible to reflect any runtime change during the uncertain small gap between transport instantiation and transport start. Also if the runtime change is not synchronized with the serializingExecutor, socketFactoryToUse().createSocket() is not thread safe with the runtime change.
There was a problem hiding this comment.
It is now final
| @@ -156,6 +158,14 @@ public final OkHttpChannelBuilder transportExecutor(@Nullable Executor transport | |||
| return this; | |||
| } | |||
|
|
|||
| /** | |||
| * Override the default {@link SocketFactory} used to create sockets. | |||
| */ | |||
| @@ -156,6 +158,17 @@ public final OkHttpChannelBuilder transportExecutor(@Nullable Executor transport | |||
| return this; | |||
| } | |||
|
|
|||
| /** | |||
| * Override the default {@link SocketFactory} used to create sockets. If the socket factory is not | |||
| * set, the value of {@link SocketFactory#getDefault} will be used. | |||
There was a problem hiding this comment.
nit: Let's hide the implementation detail. What about "If the socket factory is not set or set to null, a default one will be used"?
| @@ -463,8 +487,8 @@ SSLSocketFactory createSocketFactory() { | |||
| private final boolean usingSharedExecutor; | |||
| private final boolean usingSharedScheduler; | |||
| private final TransportTracer.Factory transportTracerFactory; | |||
| @Nullable | |||
| private final SSLSocketFactory socketFactory; | |||
| @Nullable private final SocketFactory socketFactory; | |||
There was a problem hiding this comment.
Remove @Nullable, it's just like a random NPE from any library if SocketFactory.getDefault() were null.
This is mainly aimed at allowing Android users to force the OkHttp channel to use a particular network, via providing a socket factory obtained from
android.net.Network#getSocketFactory(). The corresponding feature request from an internal user is at b/119564668.