Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a socket abstraction #8410

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

ean5533
Copy link
Collaborator

@ean5533 ean5533 commented May 14, 2024

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 ean5533 force-pushed the enelson-jwilson-pretend-to-be-okio branch from 8c9dd28 to 37f5924 Compare May 14, 2024 23:26
@@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Collaborator Author

@ean5533 ean5533 May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I tossed a @ExperimentalOkHttpApi on here

@ean5533 ean5533 force-pushed the enelson-jwilson-pretend-to-be-okio branch from 587f76c to 353c888 Compare May 14, 2024 23:39
@ean5533 ean5533 force-pushed the enelson-jwilson-pretend-to-be-okio branch from 353c888 to 7266ab7 Compare May 14, 2024 23:58
Copy link
Collaborator

@yschimke yschimke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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>())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

interface OkioSocketFactory {
fun createSocket(): OkioSocket

fun createSocket(proxy: Proxy): OkioSocket
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

connectTimeout: Int,
)

fun shutdownOutput()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


fun connect(
address: InetSocketAddress,
connectTimeout: Int,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
RealOkioServerSocketFactory(
object : DelegatingServerSocketFactory(getDefault()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants