Add a socket abstraction #8410

ean5533 · 2024-05-14T18:43:52Z

What

This PR adds a bunch of abstractions over Socket and related factories. We named the abstraction classes things like OkioSocket which telegraphs our intent to eventually upstream these into Okio. For now they'll just live in okhttp3.internal.

Why

This is the first step toward making our test infrastructure better. This will allow us to create fake sockets that integrate with TaskFaker which yield on read/write so that other tasks can get a shot.

As a motivating use case, this will allow us to fix the flaky ConnectPoolTest.connectionPreWarmingHttp2.

What's next

Implement a bunch of fakes that play nicely with TaskFaker and use them in MockHttp2Peer, MockWebServer, etc
Maybe: Use composition instead of inheritance in OkioSslSocket

ean5533 · 2024-05-14T23:26:52Z

mockwebserver/api/mockwebserver3.api

@@ -107,7 +107,7 @@ public final class mockwebserver3/MockWebServer : java/io/Closeable {
 	public final fun getProtocolNegotiationEnabled ()Z
 	public final fun getProtocols ()Ljava/util/List;
 	public final fun getRequestCount ()I
-	public final fun getServerSocketFactory ()Ljavax/net/ServerSocketFactory;
+	public final fun getServerSocketFactory ()Lokhttp3/internal/socket/OkioServerSocketFactory;


How do we feel about breaking changes to MockWebServer public methods?

Changing this new mockwebserver seems fine, we should avoid changes to okhttp3/mockwebserver/* in the deprecated package.

But I guess these APIs in the non deprecated API become experimental APIs, is that ok?

ean5533 · 2024-05-14T23:28:15Z

okhttp/api/okhttp.api

@@ -409,6 +409,7 @@ public final class okhttp3/ConnectionSpec {
 	public fun equals (Ljava/lang/Object;)Z
 	public fun hashCode ()I
 	public final fun isCompatible (Ljavax/net/ssl/SSLSocket;)Z
+	public final fun isCompatible (Lokhttp3/internal/socket/OkioSslSocket;)Z


Note: I tossed a @ExperimentalOkHttpApi on here

yschimke

I like it, but it's disruptive enough including for legacy 4.x APIs that you probably want @swankjesse input before addressing any of my suggestions.

yschimke · 2024-05-18T23:39:03Z

mockwebserver/api/mockwebserver3.api

@@ -107,7 +107,7 @@ public final class mockwebserver3/MockWebServer : java/io/Closeable {
 	public final fun getProtocolNegotiationEnabled ()Z
 	public final fun getProtocols ()Ljava/util/List;
 	public final fun getRequestCount ()I
-	public final fun getServerSocketFactory ()Ljavax/net/ServerSocketFactory;
+	public final fun getServerSocketFactory ()Lokhttp3/internal/socket/OkioServerSocketFactory;


Changing this new mockwebserver seems fine, we should avoid changes to okhttp3/mockwebserver/* in the deprecated package.

yschimke · 2024-05-18T23:40:22Z

mockwebserver/api/mockwebserver3.api

@@ -107,7 +107,7 @@ public final class mockwebserver3/MockWebServer : java/io/Closeable {
 	public final fun getProtocolNegotiationEnabled ()Z
 	public final fun getProtocols ()Ljava/util/List;
 	public final fun getRequestCount ()I
-	public final fun getServerSocketFactory ()Ljavax/net/ServerSocketFactory;
+	public final fun getServerSocketFactory ()Lokhttp3/internal/socket/OkioServerSocketFactory;


But I guess these APIs in the non deprecated API become experimental APIs, is that ok?

yschimke · 2024-05-18T23:43:19Z

mockwebserver-deprecated/api/mockwebserver.api

@@ -69,7 +69,7 @@ public final class okhttp3/mockwebserver/MockWebServer : org/junit/rules/Externa
 	public final fun -deprecated_protocols ()Ljava/util/List;
 	public final fun -deprecated_protocols (Ljava/util/List;)V
 	public final fun -deprecated_requestCount ()I
-	public final fun -deprecated_serverSocketFactory (Ljavax/net/ServerSocketFactory;)V
+	public final fun -deprecated_serverSocketFactory (Lokhttp3/internal/socket/OkioServerSocketFactory;)V


Should we hide this in this deprecated API, keep it still in terms of javax.net?

yschimke · 2024-05-18T23:44:21Z

mockwebserver-deprecated/src/main/kotlin/okhttp3/mockwebserver/MockWebServer.kt

@@ -37,7 +37,7 @@ class MockWebServer : ExternalResource(), Closeable {

  var bodyLimit: Long by delegate::bodyLimit

-  var serverSocketFactory: ServerSocketFactory? by delegate::serverSocketFactory
+  var serverSocketFactory: OkioServerSocketFactory? by delegate::serverSocketFactory


I guess I'm wondering whether dropping down to the native implementation should be a feature, and so here we could grab the real ServerSocketFactory?

yschimke · 2024-05-18T23:48:45Z

mockwebserver/src/main/kotlin/mockwebserver3/MockWebServer.kt

@@ -107,7 +109,7 @@ class MockWebServer : Closeable {
  private val taskRunner = TaskRunner(taskRunnerBackend)
  private val requestQueue = LinkedBlockingQueue<RecordedRequest>()
  private val openClientSockets =
-    Collections.newSetFromMap(ConcurrentHashMap<Socket, Boolean>())
+    Collections.newSetFromMap(ConcurrentHashMap<OkioSocket, Boolean>())


Given OkioSocket is an interface, does this weaken our safe assumptions about equals/hashcode being object reference ones?

Not sure if this means

a) documenting the equals/hashcode behaviour
b) introducing some Socket identifier?
c) changing this to a IdentityHashMap?

This is a great question. My instinct is to document that equals/hashcode are referenced-based, because I think that's the only reasonable way to think about socket equality. I think I would be surprised if sockets were considered equal based on anything than their unique state, at least by default.

Maybe, although a socket is a 4-tuple of host:port, host:port.

And equality could break with decoration used this way.

Probably documenting reference equality is right, just bouncing ideas.

yschimke · 2024-05-19T00:26:34Z

okhttp/src/main/kotlin/okhttp3/internal/socket/OkioSocketFactory.kt

+interface OkioSocketFactory {
+  fun createSocket(): OkioSocket
+
+  fun createSocket(proxy: Proxy): OkioSocket


I wonder if we want this here? Could we say that a proxy is a different OkioSocketFactory? What would we lose here?

yschimke · 2024-05-19T00:27:25Z

okhttp/src/main/kotlin/okhttp3/internal/socket/OkioSocket.kt

+    connectTimeout: Int,
+  )
+
+  fun shutdownOutput()


Would we lose much by expecting clients to do sink.close() ?

yschimke · 2024-05-19T00:31:23Z

okhttp/src/main/kotlin/okhttp3/internal/socket/OkioSocket.kt

+
+  fun connect(
+    address: InetSocketAddress,
+    connectTimeout: Int,


Should connectTimeout be a property of the OkioSocket instead? or handled via Okio Timeout?

yschimke · 2024-05-19T00:35:01Z

okhttp/src/test/java/okhttp3/URLConnectionTest.kt

-        }
-      }
+      RealOkioServerSocketFactory(
+        object : DelegatingServerSocketFactory(getDefault()) {


possibly a TODO? It would be nice to not need DelegatingServerSocketFactory and friends because of your beautiful new API.

I'm not suggesting interceptors :) More that decorating could be made simpler?

yschimke · 2024-05-19T00:38:41Z

samples/unixdomainsockets/src/main/java/okhttp3/unixdomainsockets/ClientAndServer.java

@@ -34,7 +35,9 @@ public void run() throws Exception {
    socketFile.delete(); // Clean up from previous runs.

    MockWebServer server = new MockWebServer();
-    server.setServerSocketFactory(new UnixDomainServerSocketFactory(socketFile));
+    RealOkioServerSocketFactory serverSocketFactory =
+      new RealOkioServerSocketFactory(new UnixDomainServerSocketFactory(socketFile));


possibly worth a TODO? Feels like this new API supporting other socket types would be a win?

ean5533 added 9 commits May 14, 2024 11:01

Add an OkioSocket

c6bc4ac

Jesse did stuff

9dd5a2f

Evan did stuff

ffdb6f0

Fixed all but expected failure

db0cd0c

Spotless

7526606

Move into separate files

ef360f4

Remove TaskFakerOkioSocket for now

4b84b8f

Cleanup

d1f862e

apiDump

37f5924

ean5533 force-pushed the enelson-jwilson-pretend-to-be-okio branch from 8c9dd28 to 37f5924 Compare May 14, 2024 23:26

ean5533 commented May 14, 2024

View reviewed changes

ean5533 force-pushed the enelson-jwilson-pretend-to-be-okio branch from 587f76c to 353c888 Compare May 14, 2024 23:39

Flaky test

7266ab7

ean5533 force-pushed the enelson-jwilson-pretend-to-be-okio branch from 353c888 to 7266ab7 Compare May 14, 2024 23:58

yschimke reviewed May 19, 2024

View reviewed changes

yschimke mentioned this pull request Jun 22, 2024

Reenable connectionPreWarmingHttp2 #8451

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a socket abstraction #8410

Add a socket abstraction #8410

ean5533 commented May 14, 2024

ean5533 May 14, 2024

yschimke May 18, 2024

yschimke May 18, 2024

ean5533 May 14, 2024 •

edited

Loading

yschimke left a comment

yschimke May 18, 2024

yschimke May 18, 2024

yschimke May 18, 2024

yschimke May 18, 2024

yschimke May 18, 2024

ean5533 May 20, 2024

yschimke May 20, 2024

yschimke May 19, 2024

yschimke May 19, 2024

yschimke May 19, 2024

yschimke May 19, 2024

yschimke May 19, 2024

yschimke May 19, 2024

Add a socket abstraction #8410

Are you sure you want to change the base?

Add a socket abstraction #8410

Conversation

ean5533 commented May 14, 2024

What

Why

What's next

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ean5533 May 14, 2024 • edited Loading

Choose a reason for hiding this comment

yschimke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ean5533 May 14, 2024 •

edited

Loading