Add support for HTTP/1.1 protocol upgrades by gesellix · Pull Request #8849 · lysine-dev/okhttp

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

+  // 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?

I wonder if you can test any of this with container-tests?

https://github.com/square/okhttp/blob/master/container-tests/src/test/java/okhttp3/containers/SocksProxyTest.kt#L81

Thanks, I wasn't aware of those. I'll look into it!

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

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

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

+  /**
+   * 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.

My understanding is that on a protocol upgrade, OkHttp should hand off the okio.Socket to the application layer, and it can do whatever new protocol it wants on top. Our responsibility for the upgrade ends and handing off the socket.

Websockets might be the only exception?

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 · 2025-07-07T06:49:53Z

Thanks for looking into it.
I can merge the current main branch or rebase on top of it to resolve the conflicts, if you like.

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

gesellix · 2025-07-25T09:59:55Z

The branch has been rebased now (I had to make it twice to make spotless happy).

swankjesse · 2025-07-25T13:50:23Z

I think running ./gradlew apiDump will fix the test failure.

swankjesse · 2025-07-25T14:00:52Z

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


My understanding is that on a protocol upgrade, OkHttp should hand off the okio.Socket to the application layer, and it can do whatever new protocol it wants on top. Our responsibility for the upgrade ends and handing off the socket.

swankjesse · 2025-07-25T14:16:02Z


  val responseCode = code
-  if ((responseCode < HTTP_CONTINUE || responseCode >= 200) &&
+  if ((responseCode < HTTP_CONTINUE || responseCode == HTTP_SWITCHING_PROTOCOLS || responseCode >= 200) &&


I don’t think I agree that a protocol change is a body? This is used to drive stuff like the gzip interceptor, which seems inappropriate here?

The protocol change itself doesn't promise a body, but would depend on the actual protocl after a successful upgrade.
I think this change was some kind of workaround to run into the "newUnknownLengthSource(response.request.url)" case at https://github.com/gesellix/okhttp/blob/0984bc3fc5d41c4814b953194d10e5ee77361f7d/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http1/Http1ExchangeCodec.kt#L145
Maybe there's a better way?

gesellix · 2025-07-28T10:39:35Z

The pull request will be continued at #8969

gesellix mentioned this pull request Jun 10, 2025

Support HTTP/1.1 protocol upgrades #5874

Closed

gesellix commented Jun 10, 2025

View reviewed changes

gesellix mentioned this pull request Jun 10, 2025

Connection Upgrade for HTTP1 gesellix/okhttp#4

Closed

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

yschimke reviewed Jun 16, 2025

View reviewed changes

Comment thread mockwebserver/src/main/kotlin/mockwebserver3/MockWebServer.kt Outdated

yschimke reviewed Jun 16, 2025

View reviewed changes

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

yschimke reviewed Jun 16, 2025

View reviewed changes

Comment thread okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/RealConnection.kt Outdated

yschimke reviewed Jun 16, 2025

View reviewed changes

Comment thread okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/CallServerInterceptor.kt Outdated

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

gesellix added 2 commits July 25, 2025 10:05

Give the CountDownLatch something to count

889b147

Fix typo

9009b05

gesellix force-pushed the i5874 branch from b1f57d2 to e245aa1 Compare July 25, 2025 09:11

Connection Upgrade for HTTP1 status code 101

79e4373

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

gesellix force-pushed the i5874 branch from e245aa1 to 79e4373 Compare July 25, 2025 09:58

gesellix commented Jul 25, 2025

View reviewed changes

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

swankjesse reviewed Jul 25, 2025

View reviewed changes

Comment thread mockwebserver/src/main/kotlin/mockwebserver3/MockWebServer.kt Outdated

swankjesse reviewed Jul 25, 2025

View reviewed changes

gesellix added 7 commits July 25, 2025 21:08

use socket.asOkioSocket()

6e337e3

add socket to api

2d4d678

remove the specific check on tcp connection upgrades

ffce1d1

create a dedicated HttpUpgradesTest

335d69e

chore

dd09d8d

add more test cases

f2daee6

spotlessApply

ab7c625

gesellix mentioned this pull request Jul 28, 2025

Add support for HTTP/1.1 connection upgrades #8969

Merged

gesellix closed this Jul 28, 2025

gesellix deleted the i5874 branch July 28, 2025 16:31

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gesellix commented Jun 14, 2025

Uh oh!

Uh oh!

Uh oh!

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!

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!

gesellix commented Jul 7, 2025

Uh oh!

gesellix commented Jul 25, 2025

Uh oh!

Uh oh!

swankjesse commented Jul 25, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gesellix commented Jul 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

gesellix commented Jun 10, 2025 •

edited

Loading

yschimke commented Jun 16, 2025 •

edited

Loading