Add support for HTTP/1.1 protocol upgrades #8849

gesellix · 2025-06-10T18:52:42Z

Based on the discussion in #5874

See RFC 7230 section 6.7 and RFC 9110 section 15.2.2.

The first two commits are not related and can be submitted as separate pull request.

This is only a draft to continue the discussion started in #5874 and not ready for an actual merge, yet.

gesellix · 2025-06-10T18:56:25Z

okhttp/src/jvmTest/kotlin/okhttp3/CallTest.kt

+  // https://learn.microsoft.com/en-us/windows/win32/ipc/named-pipe-client
+  // docker run --rm -it --name attach alpine:edge top -b
+  @Test
+  fun upgradeConnection() {


I'm not happy with this one, maybe you have better ideas how to test the connection upgrade?

See https://datatracker.ietf.org/doc/html/rfc9110#name-101-switching-protocols

gesellix · 2025-06-14T09:43:34Z

This is now rebased on the current master to use the new okio.Socket.

yschimke · 2025-06-16T13:25:12Z

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

+    if (response.socketHandler != null) {
+      response.socketHandler.handle(
+        object : okio.Socket {
+          override val source: BufferedSource


I'm not clear who can access this as a BufferedSource for an anonymous object impl. Wouldn't clients still have to Buffer again to be sure?

I guess I don't understand your question. Are you implying that this should be changed to a non-buffered Source?

Yeah, are you relying on this being buffered here? Or also when used it's applied

I don't rely on this being buffered, so I commited a cleanup 👍

yschimke · 2025-06-16T13:25:50Z

mockwebserver/src/main/kotlin/mockwebserver3/internal/duplex/MockSocketHandler.kt

@@ -82,7 +82,7 @@ class MockSocketHandler : SocketHandler {
  @JvmOverloads
  fun sendResponse(
    s: String,
-    responseSent: CountDownLatch = CountDownLatch(0),
+    responseSent: CountDownLatch = CountDownLatch(1),


Is this fixing an unrelated bug?

According to the Javadoc, a CountDownLatch with a zero value doesn't have any effect. I didn't experience a bug.

This is unrelated to the current pull request, though. I referred to this one as one of the two commits which are both unrelated and can easily be moved to a dedicated pull request.

yschimke · 2025-06-16T13:27:54Z

okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/RealConnection.kt

+        get() = socket.sink().buffer()
+
+      override fun cancel() {
+        socket.sink().flush()


Seems to contradict the API

https://github.com/square/okio/blob/bcd25f94b63723a9e90cb74d6b76fff16acfed34/okio/src/commonMain/kotlin/okio/Socket.kt#L44

Fail any in-flight and future operations.

You're right, I didn't check the api thoroughly. I'm going to remove this one.

This has been removed in RealConnection as well as in the MockWebServer.

... is it correct and sufficient to cancel the Exchange?

yschimke · 2025-06-16T13:43:41Z

okhttp/src/commonJvmAndroid/kotlin/okhttp3/Response.kt

+  /**
+   * Non-null if this response is a successful upgrade ...
+   */
+  @get:JvmName("socket") val socket: Socket?,


I'm a bit unclear why this is enough to consider upgrades handled. Is this only for TCP? or do you think that all protocols would be handled as a Socket?

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Upgrade#upgrade_header_with_multiple_protocols

I must admit that I only considered TCP upgrades and ignored other options. I suppose that the socket will only be necessary (and non-null) for upgrades which are not supported through another implementation (e.g. HTTP2). Returning the plain Socket might be considered as a generic way of supporting any protocol. That said: I'm unsure if this would make sense for OkHttp in general or if you prefer to keep supported protocols explicitly stated anywhere.
So, would it be enough to improve the Javadoc to be more specific?

I think Jesse should weigh in. I don't think this is a simple feature to add especially given your TCP case is a poorly specced docker variant of a more general feature.

yschimke · 2025-06-16T13:46:05Z

okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/CallServerInterceptor.kt

+          if (code == HTTP_SWITCHING_PROTOCOLS &&
+            "upgrade".equals(response.request.header("Connection"), ignoreCase = true) &&
+            "upgrade".equals(response.header("Connection"), ignoreCase = true) &&
+            "tcp".equals(response.request.header("Upgrade"), ignoreCase = true) &&


The PR title feels like it's about all possible protocols, but in practice seems tied to solving TCP specifically for docker?

Is there a spec for this TCP upgrade?

You're right, I wasn't aware of the more general possibilities for a connection upgrade, and the case for Websockets hasn't changed with my pull request.

My current aim is in fact to add support for TCP upgrades, because my use case is driven by the Docker engine api.
There's no spec for the TCP upgrade per se, at least not known to me.

Given the issue at #5874 I would suggest to start with support for tcp and websocket. Other protocols might be added in the future.

yschimke · 2025-06-16T13:58:30Z

I'm struggling to find any implementations of the TCP upgrade in popular clients like Curl, Netty, Requests etc.

Can you link to any?

And wondering whether this really needs a more thorough implementation of the spec, which allows for negotation.

Example from Netty https://github.com/netty/netty/blob/42d83b8081ab3d018f5d7611973e5d361cdd28d1/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java#L40

gesellix · 2025-06-16T18:49:38Z

I'm struggling to find any implementations of the TCP upgrade in popular clients like Curl, Netty, Requests etc.

Can you link to any?

And wondering whether this really needs a more thorough implementation of the spec, which allows for negotation.

Example from Netty https://github.com/netty/netty/blob/42d83b8081ab3d018f5d7611973e5d361cdd28d1/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java#L40

There is a use case with the Apache Http Client, so not exactly an example in a client: https://github.com/docker-java/docker-java/blob/20f08311d1613d8819c4548f34046b0557a43117/docker-java-transport-httpclient5/src/main/java/com/github/dockerjava/httpclient5/HijackingHttpRequestExecutor.java
The official Docker Clients (docker-client, docker-py) implement the TCP upgrade described at https://docs.docker.com/reference/api/engine/version/v1.50/#tag/Container/operation/ContainerAttach

Looking at https://datatracker.ietf.org/doc/html/rfc9110#field.upgrade there's at least the Expect header mentioned. Is this what you mean by negotiation?

As far as I understand the Netty implementation, it handles the connection upgrade more or less by "passing through" and only checking response headers for the requested protocol. I didn't dig deeper into how exactly it would allow the client to handle the upgraded connection.
Maybe OkHttp could select some of well-known or "popular" protocols (e.g. Websocket) for specific upgrade handling and fall back to the more generic Socket for unknown or non-officially supported protocols?

yschimke · 2025-06-16T19:24:18Z

Thanks, that's useful.

Yeah there are maybe three paths

special case each transport we support (TCP, websocket)
websocket or okio.Socket (TCP + other)
design a proper upgrade API

gesellix · 2025-07-03T18:20:07Z

Given the release of OkHttp 5, and this one probably changing the api, is there still a chance to have some kind of minimal implementation for the use case of OkHttp as client for a Docker Engine and the tcp upgrade? I guess there's no other chance of using OkHttp without changing its api? I'm not afraid of maintaining some kind of extension or module so that you wouldn't have to officially support such a feature, but I have no idea how I could implement such a module without maintaining a full OkHttp fork by myself.
A minimal support for specific use cases of the HTTP/1.1 protcol upgrade would be really great. I'm such a big fan of OkHttp :)

yschimke · 2025-07-03T18:35:34Z

@gesellix I think it's worth supporting, but I'd like some other opinions on it also.

I guess what is complicated here is the PR is a partial implementation that covers a non-standardised but valid case. While the standardized cases aren't supported.

gesellix · 2025-07-03T18:45:07Z

So, how much of a standard use case (which ones?) would be required to put some weight or clarity into it? Maybe this is not the right timing to push such a feature and I assume that you don't have other users asking for anything with a connection upgrade (on top of existing implementations for Websocket and HTTP/2).

Do you have an indication which standard use cases make sense? I could try to prepare the required changes either with this pull request or in a separate one. I might only need some guidance on the interfaces and testing if that would work for you.

yschimke · 2025-07-03T19:03:32Z

If we have a clean mechanism, and this is currently the only supported protocol, but we can add more. That is probably ok.

But if the public API of things like Response change in a way that has a bunch of assumptions that only hold for this TCP docker case, it would be a mistake.

@swankjesse if you have opinions, feel free to weigh in.

swankjesse · 2025-07-03T23:36:51Z

I’d like to ship this.

I’d like to review this PR and the RFCs to figure out exactly what’s best for our next step and I intend to do so.

gesellix mentioned this pull request Jun 10, 2025

Support HTTP/1.1 protocol upgrades #5874

Open

gesellix commented Jun 10, 2025

View reviewed changes

gesellix mentioned this pull request Jun 10, 2025

Connection Upgrade for HTTP1 gesellix/okhttp#4

Open

gesellix added 4 commits June 14, 2025 11:38

Give the CountDownLatch something to count

b15fb15

Fix typo

d34b5bd

Connection Upgrade for HTTP1 status code 101

b4423b9

See https://datatracker.ietf.org/doc/html/rfc9110#name-101-switching-protocols

Rebase to current master

3259c40

gesellix force-pushed the i5874 branch from b86da3b to 3259c40 Compare June 14, 2025 09:39

yschimke reviewed Jun 16, 2025

View reviewed changes

review suggestions

b1f57d2

yschimke added this to the 5.x milestone Jun 22, 2025

Add support for HTTP/1.1 protocol upgrades #8849

Are you sure you want to change the base?

Add support for HTTP/1.1 protocol upgrades #8849

Conversation

gesellix commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gesellix commented Jun 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yschimke commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gesellix commented Jun 16, 2025

Uh oh!

yschimke commented Jun 16, 2025

Uh oh!

gesellix commented Jul 3, 2025

Uh oh!

yschimke commented Jul 3, 2025

Uh oh!

gesellix commented Jul 3, 2025

Uh oh!

yschimke commented Jul 3, 2025

Uh oh!

swankjesse commented Jul 3, 2025

Uh oh!

Uh oh!

gesellix commented Jun 10, 2025 •

edited

Loading

yschimke commented Jun 16, 2025 •

edited

Loading