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

[Early WIP] Websockets over HTTP/2 #4014

Closed
wants to merge 12 commits into from

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented May 20, 2018

Towards https://datatracker.ietf.org/doc/draft-ietf-httpbis-h2-websockets/?include_text=1

Steps - working on step 2 currently

  1. Introduce an API for UpgradeHandler
  2. Refactor existing Websocket code to cleanly work with UpgradeHandler for HTTP/1.1
  3. Introduce HTTP/1.1 upgrade flow
  4. Implement HTTP/2 upgrade flow

Public API changes

  1. None - but internal APIs to support specific upgrade over HTTP/1.1 or HTTP/2

TODO

  1. fix clientCancelsIfCloseIsNotAcknowledged test and 5 other ping tests disabled - haven't debugged

@yschimke
Copy link
Collaborator Author

Main API

public interface UpgradeHandler<T> {
  T connect(OkHttpClient client);

  boolean supportsHttp11();

  boolean supportsHttp2();

  /**
   * https://www.iana.org/assignments/http-upgrade-tokens/http-upgrade-tokens.xhtml
   */
  String connectProtocol();

  Request addUpgradeHeaders(Request upgradeRequest);

  void failConnect(Response response, IOException e) throws IOException;
}

@JakeWharton
Copy link
Collaborator

The public API implications here are pretty devastating. Do we really need to alter the surface area so much?

@yschimke
Copy link
Collaborator Author

I can implement it with all internal changes for now. But I’d love to be able to use with other external upgraded protocols provided by libraries. I can make that a completely separate later PR and discussion

@JakeWharton
Copy link
Collaborator

JakeWharton commented May 22, 2018 via email

@yschimke
Copy link
Collaborator Author

yschimke commented May 23, 2018

You had me at "The public API implications"

image

@swankjesse
Copy link
Collaborator

I think it’d be great to get this working. I’m using a lot of web sockets in one of my current projects and this could make things a lot smoother.

How does the client indicate that we should attempt HTTP/2? Do we need to set policy? I think that’s the trickiest part of the whole business.

@yschimke
Copy link
Collaborator Author

yschimke commented May 30, 2018

Good question. I was planning to attempt HTTP/2 once per client/host when there is existing connection. Then remember failure. Default to http/1.1 for new connections.

I think getting in the basic support first is useful. Before widespread adoption and hopefully to help foster it.

OkHttp client API isn’t really helpful for per request or per host changes.

Any suggestions? It's not public API yet - policy implies it would be.

@swankjesse
Copy link
Collaborator

My preference is that eventually it just works: if the server can do HTTP/2 web sockets we use ’em.

To get there I think we probably do a phased migration in:

  • initially it’s off by default with some hint to turn it on (maybe a header on the Request?)
  • later it’s on by default with some hint to turn it off (another header?)

In either case I think we get something like RetryAndFollowUpInterceptor to fallback to HTTP/1 if HTTP/2 doesn’t work.

@yschimke yschimke added this to the 3.12 milestone Jul 11, 2018
@yschimke yschimke mentioned this pull request Sep 14, 2018
@yschimke
Copy link
Collaborator Author

No time to fix up at the moment.

@yschimke yschimke closed this Jan 20, 2019
@yschimke yschimke deleted the websocket_http2 branch June 22, 2019 10:48
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.

3 participants