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

gh-1019: Implemented prior knowledge approach to h2c #3873

Merged
merged 23 commits into from Feb 26, 2018

Conversation

@mjpitz
Copy link
Contributor

commented Feb 20, 2018

Needs a unit test, but I wanted to get the idea out there of how easy this could be.

An Address's List comes from the initial configuration. If someone specifies the H2C protocol in that list, then we assume prior knowledge since it requires users to specify the protocol in their configuration.

I tried to wire a boolean attribute through the constructor chain, but that got really gross. This seemed like an easier approach with a lot fewer changes in code.

@swankjesse

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

I’m reluctant to implement support for H2C because it’s a cleartext protocol and cleartext is dead to me.

@jjshanks

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@swankjesse While understandable we have a pretty reasonable use case. Transparent TLS via a service mesh. We have clients that use Retrofit and Feign but can't use h2c to talk to localhost without this kind of change.

@swankjesse

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

Gotcha. So cleartext doesn’t leave the host. That’s not so bad.


// this implementation takes the "prior knowledge" approach to the spec
// https://tools.ietf.org/html/rfc7540#section-3.4
if (route.address().protocols().contains(Protocol.H2C)) {

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 21, 2018

Collaborator

I suggest we change

https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/OkHttpClient.java#L854-L856

To allow H2C on it's own, but only on it's own, no fallback to HTTP/1.1.

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 22, 2018

Author Contributor

Done. This ended up cleaning up some of the logic in the establishment.

@yschimke

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2018

I'm a big fan of this, have hit this a couple of times before during development and ended up using Netty.

@yschimke

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2018

To be clear, my main concern is this makes a client that is single purpose. So we should probably only allow H2C as the single supported protocol for a client. Otherwise this client instance will be mostly usable for https but broken for most external plaintext traffic.

It seems better to be really explicit that the client is single purpose.

// when using h2c prior knowledge, no other protocol should be supported.
throw new IllegalArgumentException("protocols containing h2c cannot use other protocols:"
+ uniqueProtocols);
} else if (!uniqueProtocols.contains(Protocol.HTTP_1_1)) {

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 22, 2018

Author Contributor

bug: this needs to check if uniqueProtocols does not contain H2c

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 22, 2018

Collaborator

This is checked implicitly via the if case above

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 23, 2018

Author Contributor

Actually it's not. In the event that someone specifies only H2C, then the second if check evaluates to false, triggering this else if. Caught it in one of the unit tests I was writing.

* Cleartext implementation of HTTP2. This enumeration exists for the "prior knowledge" upgrade
* semantic supported by the protocol.
*/
H2C("h2c"),

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 22, 2018

Author Contributor

bug: this needs to be added to the static get method.

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 23, 2018

Author Contributor

Resolved

// Validate that the list has everything we require and nothing we forbid.
if (!protocols.contains(Protocol.HTTP_1_1)) {
throw new IllegalArgumentException("protocols doesn't contain http/1.1: " + protocols);
final Set<Protocol> uniqueProtocols = new HashSet<>(protocols);

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 22, 2018

Collaborator

@swankjesse has stated a preference for LinkedHashSet makes these a lot more predictable in tests, e.g. checking the exception message. But generally you could probably just check the contains(H2C) and size > 1 on the list and fold in duplicates as bad input. But this isn't super critical code path, so that's minor.

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 23, 2018

Author Contributor

Good catch. -It wasn't super clear if the list of protocols was treated as a prioritized.- Adding to preserve ordering since it's in the Javadoc that order is expected to be preserved.


// Assign as an unmodifiable list. This is effectively immutable.
this.protocols = Collections.unmodifiableList(protocols);
this.protocols = Collections.unmodifiableList(new ArrayList<>(uniqueProtocols));

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 22, 2018

Collaborator

I think changing the selected protocol because of hashing could affect the ALPN selection IIRC. Not so much an issue without SPDY, but undesirable.

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 23, 2018

Author Contributor

Yeah. I think that change went a little too far. Going to revert back to using just the list and suffer the minimal performance cost.

@mjpitz mjpitz force-pushed the mjpitz:jpitz/gh-1019 branch from d9bdbb6 to 63b6bc0 Feb 23, 2018

@mjpitz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2018

Do y'all prefer rebase or merge when conflicts arise? I didn't see a CONTRIBUTING guide anywhere.

I tend to do merges, but don't have a preference for either way.

@JakeWharton

This comment has been minimized.

Copy link
Collaborator

commented Feb 23, 2018

Jaye Pitzeruse and others added some commits Feb 23, 2018

Jaye Pitzeruse
gh-1019: added reference to prior knowledge on the protocol enumerati…
…on and added a concrete check on the protocols list.
@mjpitz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2018

Working checkpoint. I tested this against a service I have that supports H2C out of box. Started to make modifications to the MockWebServer, but that might be a bit too ambitious. I tried putting in a shortcut, but that didn't seem to work. I'll try a few more things over the next couple of days

@mjpitz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2018

Alright, I managed to get the MockWebServer working and pulled the test cases from HttpOverHttp2Test up to an Http2TestBase that can be shared by both the cleartext and TLS flavors.

Added to this review because creating another one would effectively re-diff this entire set.

/**
* Test H2C, the cleartext implementation of HTTP/2.
*
* @author jpitz

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 23, 2018

Collaborator

strip author tags

@@ -124,6 +124,9 @@ public Http2Codec(OkHttpClient client, Interceptor.Chain chain, StreamAllocation
@Override public Response.Builder readResponseHeaders(boolean expectContinue) throws IOException {
List<Header> headers = stream.takeResponseHeaders();
Response.Builder responseBuilder = readHttp2HeadersList(headers);
if (client.protocols().contains(Protocol.H2C)) {

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 23, 2018

Collaborator

why set it here and not inside readHttp2HeadersList?

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 23, 2018

Author Contributor

That method was public static and so I would need to thread the value through the method signature, causing a breaking API change.

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 24, 2018

Author Contributor

This could actually be cached on the class. No need to do the linear scan all the time.

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 24, 2018

Author Contributor

Alternatively, another attribute could be added to the OkHttpClient and that can be a cached referenced for the lifetime of the client

@@ -260,9 +260,14 @@ private void connectSocket(int connectTimeout, int readTimeout, Call call,

private void establishProtocol(ConnectionSpecSelector connectionSpecSelector,
int pingIntervalMillis, Call call, EventListener eventListener) throws IOException {
if (route.address().sslSocketFactory() == null) {
protocol = Protocol.HTTP_1_1;
if (route.address().protocols().contains(Protocol.H2C)) {

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 23, 2018

Collaborator

It seems like this check could be a subset of the "if (route.address().sslSocketFactory() == null) " case

meaning 0 cost in the more common https case, and clearer that this is a special case of clear text. Thoughts?

@@ -844,15 +844,25 @@ public Builder dispatcher(Dispatcher dispatcher) {
* HTTP/1.1} only. If the server responds with {@code HTTP/1.0}, that will be exposed by {@link
* Response#protocol()}.
*
* @param protocols the protocols to use, in order of preference. The list must contain {@link
* Protocol#HTTP_1_1}. It must not contain null or {@link Protocol#HTTP_1_0}.
* @param protocols the protocols to use, in order of preference. If the list contains {@link

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 23, 2018

Collaborator

Could we reword this to start with the normal case of HTTP_1_1. As it is has a very H2C centric view of the world.

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 23, 2018

Author Contributor

I'll take another crack. I started trying to word it that way, but ran into issues with clarity.

* Base test cases for HTTP2 communication. These tests are executed for both H2C and HTTP2
* based requests. For implementation specific tests, include in the appropriate test case.
*
* This code originally lived in the HttpOverHttp2Test class, but was abstracted out for use

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 23, 2018

Collaborator

I don't think this history is needed. We have git for that.

}

@Test
public void testProtocol() throws Exception {

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 23, 2018

Collaborator

It feels like these tests from here onwards would be more natural in ProtocolTest (new), MockWebserverTest and OkHttpClientTest. Thoughts?

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 23, 2018

Author Contributor

Ah. I looked for a ProtocolTest and there wasn't one. Didn't look again after handling the merge. I'll move them.


@Before
public void setUp() throws Exception {
super.setUp(

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 23, 2018

Collaborator

It feels like this could be handled without as much major surgery with a @parameterized test. Not sure which is generally favoured, but I try to avoid subclassing in case there is another dimension of freedom to the tests in the future e.g. Platform.

But interested to hear input from others and yours.

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 23, 2018

Collaborator

It also means you can't go to a single test and run it from within Http2TestBase.

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 23, 2018

Author Contributor

Huh.. This week I learned. Didn't know about Parameterized builds in JUnit. This is exactly what I am looking for.

Jaye Pitzeruse added some commits Feb 23, 2018

Jaye Pitzeruse
gh-1019: switched over to using a parameterized unit test and moved r…
…emaining test methods into appropriate classes

@mjpitz mjpitz force-pushed the mjpitz:jpitz/gh-1019 branch from e2292c9 to b5f36eb Feb 23, 2018

@mjpitz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2018

@yschimke : master doesn't appear to have a ProtocolTest yet. Where is it?

.build();
}


This comment has been minimized.

Copy link
@yschimke

yschimke Feb 24, 2018

Collaborator

nit: double line

@Parameters
public static Collection<Object[]> data() {

return Arrays.asList(new Object[][] {

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 24, 2018

Collaborator

indentation looks off

@yschimke

This comment has been minimized.

Copy link
Collaborator

commented Feb 24, 2018

Looking good to me, only nits and formatting.

@@ -456,4 +457,22 @@
RecordedRequest request = server.takeRequest();
assertEquals("request", request.getBody().readUtf8());
}

@Test(expected = IllegalArgumentException.class)

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 24, 2018

Collaborator

Same as previous comment, OkHttp tests favour explicit catch and check exception message over annotation expected tests.

@yschimke
Copy link
Collaborator

left a comment

Care to update the description, remove "quick stab", before we merge ?

@mjpitz mjpitz changed the title gh-1019: quick stab at a prior knowledge approach to h2c gh-1019: Implemented prior knowledge approach to h2c Feb 24, 2018

@mjpitz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2018

@yschimke : done. any idea what's going on with the travis test? I'd prefer to have a green build before pushing forward

@swankjesse
Copy link
Member

left a comment

This is great.

@@ -420,7 +428,7 @@ public void processConnection() throws Exception {
return;
}
socket = sslSocketFactory.createSocket(raw, raw.getInetAddress().getHostAddress(),
raw.getPort(), true);
raw.getPort(), true);

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

nit: indent

@@ -436,6 +444,10 @@ public void processConnection() throws Exception {
protocol = protocolString != null ? Protocol.get(protocolString) : Protocol.HTTP_1_1;
}
openClientSockets.remove(raw);
} else if (protocols.contains(Protocol.H2C)) {
// force use of H2c

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

nit: I don’t think the comment is necessary!

}
}

@Test public void testH2COkHttpClientConstructionSuccess() {

This comment has been minimized.

Copy link
@swankjesse
}

@Test public void testH2COkHttpClientConstructionSuccess() {
final OkHttpClient okHttpClient = new OkHttpClient.Builder()

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

nit: final isn’t necessary

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 25, 2018

Author Contributor

habbit. we prefer having every thing be final.

@@ -0,0 +1,34 @@
package okhttp3;

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

nit: copyright header

This comment has been minimized.

Copy link
@mjpitz

mjpitz Feb 25, 2018

Author Contributor

added

public final class HttpOverHttp2Test {
private static final Logger http2Logger = Logger.getLogger(Http2.class.getName());
private static final SslClient sslClient = SslClient.localhost();

@Parameters

This comment has been minimized.

Copy link
@swankjesse
@@ -1147,6 +1183,8 @@ private int countFrames(List<String> logs, String message) {
* <p>This test uses proxy tunnels to get a hook while a connection is being established.
*/
@Test public void concurrentHttp2ConnectionsDeduplicated() throws Exception {
if (protocol == Protocol.H2C) return; // only run for http2

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

yeah, I think you want assumeTrue() here

@@ -261,8 +261,15 @@ private void connectSocket(int connectTimeout, int readTimeout, Call call,
private void establishProtocol(ConnectionSpecSelector connectionSpecSelector,
int pingIntervalMillis, Call call, EventListener eventListener) throws IOException {
if (route.address().sslSocketFactory() == null) {
protocol = Protocol.HTTP_1_1;
if (route.address().protocols().contains(Protocol.H2C)) {

This comment has been minimized.

Copy link
@swankjesse
private void startHttp2(int pingIntervalMillis) throws IOException {
socket.setSoTimeout(0); // HTTP/2 connection timeouts are set per-stream.
http2Connection = new Http2Connection.Builder(true)
.socket(socket, route.address().url().host(), source, sink)

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

supernit: +4 on wrapped lines


public Http2Codec(OkHttpClient client, Interceptor.Chain chain, StreamAllocation streamAllocation,
Http2Connection connection) {
this.client = client;
this.chain = chain;
this.streamAllocation = streamAllocation;
this.connection = connection;

// cache this so we don't do linear scans on every call

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

this comment is misleading. “linear scans” is on a list of a very small constant size.

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

(I think I’d just omit the comment)

public final class HttpOverHttp2Test {
private static final Logger http2Logger = Logger.getLogger(Http2.class.getName());
private static final SslClient sslClient = SslClient.localhost();

@Parameters

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 25, 2018

Collaborator

Can you change this so the IDE makes it clear what is running

@Parameters(name="{2}")

image

@yschimke

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2018

Running fine locally in both IDE and "mvn test", so the JVM died during HttpOverHttp2 is baffling

$ mvn test -Dtest=HttpOverHttp2Test -DfailIfNoTests=false

I'll dig into why it's failing, particularly as I pushed you towards the parameterised test, which is a likely suspect.

@yschimke

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2018

Failing with

Uncaught exception in OkHttp thread "Thread-12"
java.util.concurrent.RejectedExecutionException: Task okhttp3.internal.http2.Http2Connection$6@57e4c31e rejected from java.util.concurrent.ThreadPoolExecutor@12e64786[Shutting down, pool size = 1, active threads = 0, queued tasks = 0, completed tasks = 1]
	at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(ThreadPoolExecutor.java:2063)
	at java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:830)
	at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1379)
	at okhttp3.internal.http2.Http2Connection.pushResetLater(Http2Connection.java:907)
	at okhttp3.internal.http2.Http2Connection$ReaderRunnable.rstStream(Http2Connection.java:691)
	at okhttp3.internal.http2.Http2Reader.readRstStream(Http2Reader.java:242)
	at okhttp3.internal.http2.Http2Reader.nextFrame(Http2Reader.java:137)
	at okhttp3.internal.http2.Http2Connection$ReaderRunnable.execute(Http2Connection.java:608)
	at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
	at java.lang.Thread.run(Thread.java:748)
Last test to start was: serverSendsPushPromise_GET[h2c](okhttp3.internal.http2.HttpOverHttp2Test)
The command "mvn test -e -X -Dtest=HttpOverHttp2Test -DfailIfNoTests=false; cat okhttp-tests/target/surefire-reports/*" exited with 0.
@yschimke

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2018

It's something wacky with reuse of clients and mockwebserver instances and/or Http2Connections (these are used separately in both client and MockWebServer). Anyway, the shutdown of pushExecutor causes uncaught exceptions which we barf on.

I suspect a convergence of

  1. This PR now has multiple tests using push promises
  2. The tests run slightly differently in travis CI and locally and Intellij
  3. An existing timing bug with RejectedExecutionException thrown after shutdown of a connection
  4. An existing test bug with MockWebserver or more likely client usage e.g. we are actually sharing connections across tests?
  5. InstallUncaughtExceptionHandlerListener should ideally not System.exit() the test runner
  6. I changed logging recently to hide the output of InstallUncaughtExceptionHandlerListener by default
Jaye Pitzeruse
@yschimke
Copy link
Collaborator

left a comment

I think we will need to resolve the problems before landing this

e.g. https://github.com/square/okhttp/pull/3888/files

private static final SslClient sslClient = SslClient.localhost();

@Parameters(name = "{2}")
public static Collection<Object[]> data() {

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 25, 2018

Collaborator

This makes the client instances used across test runs. Can we change this to

@Parameters(name = "{0}") 
public static Collection<Protocol> data() {
   return Arrays.asList(Protocol.H2C, Protocol.HTTP_2);
}
private String scheme;
private Protocol protocol;

public HttpOverHttp2Test(OkHttpClient client, String scheme, Protocol protocol) {

This comment has been minimized.

Copy link
@yschimke

yschimke Feb 25, 2018

Collaborator
  public HttpOverHttp2Test(Protocol protocol) {
    this.protocol = protocol;
    this.client = protocol == Protocol.HTTP_2 ? buildClient() : buildH2cClient();
    this.scheme = protocol == Protocol.HTTP_2 ? "https" : "http";
  }

  private static OkHttpClient buildH2cClient() {
    return defaultClient().newBuilder()
        .connectionPool(new ConnectionPool())
        .protocols(Arrays.asList(Protocol.H2C))
        .build();
  }

  private static OkHttpClient buildClient() {
    return defaultClient().newBuilder()
        .protocols(Arrays.asList(Protocol.HTTP_2, Protocol.HTTP_1_1))
        .dns(new SingleInetAddressDns())
        .sslSocketFactory(sslClient.socketFactory, sslClient.trustManager)
        .hostnameVerifier(new RecordingHostnameVerifier())
        .connectionPool(new ConnectionPool())
        .build();
  }

then in tear down

  @After public void tearDown() throws Exception {
    Authenticator.setDefault(null);
    http2Logger.removeHandler(http2Handler);
    http2Logger.setLevel(previousLevel);

    client.connectionPool().evictAll();
  }

Jaye Pitzeruse added some commits Feb 25, 2018

Jaye Pitzeruse
gh-1019: simplified the constructor to the unit test and evicting all…
… connections from the pool in teardown
Jaye Pitzeruse
Jaye Pitzeruse
@@ -223,6 +227,10 @@ public void setProtocols(List<Protocol> protocols) {
this.protocols = protocols;
}

public List<Protocol> protocols() {
return Collections.unmodifiableList(protocols);

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

when we set the protocols we call Util.immutableList() so I think unmodifiableList() is not necessary

new OkHttpClient.Builder()
.protocols(Arrays.asList(Protocol.H2C, Protocol.HTTP_1_1));
fail("When H2C is specified, no other protocol can be specified");
} catch (final IllegalArgumentException e) {

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

nit: more finals

This comment has been minimized.

Copy link
@swankjesse

swankjesse Feb 25, 2018

Member

also I really like expected instead of e for these

@yschimke

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2018

This push failure happened on master https://api.travis-ci.org/v3/job/346125271/log.txt

So I suspect it is an existing problem, unrelated to this test now. The reused static clients you had in an earlier version would have made it a lot more noticeable.

@mjpitz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

Alright, I believe all feedback has been address at this point. Let me know if theres anything else y'all need from me 😄

@yschimke yschimke merged commit 9a6f88d into square:master Feb 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@swankjesse

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

Thanks @jpitz. Very nice first PR!

@mjpitz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.