More efficient and correct H1C-to-H2C upgrade #107
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trustin
force-pushed
the
h2c_upgrade_efficiency
branch
from
e8293fe
to
a822681
Compare
trustin
changed the title
trustin
force-pushed
the
h2c_upgrade_efficiency
branch
from
a822681
to
2c5a431
Compare
| synchronized (cache) { | ||
| CacheEntry e = cache.get(key); | ||
| if (e == null) { | ||
| e = new CacheEntry(); |
There was a problem hiding this comment.
return cache.computeIfAbsent(unused -> new CacheEntry())
trustin
force-pushed
the
h2c_upgrade_efficiency
branch
from
2c5a431
to
f8272d5
Compare
| } | ||
| } | ||
|
|
||
| public static void clear() { |
There was a problem hiding this comment.
This seems to be only for testing? If so let's document that (especially since it's public).
There was a problem hiding this comment.
Ugh, forgot to document the public methods properly. Will fix next week.
There was a problem hiding this comment.
It's not only for testing though. I guess a user might want to clear the cache explicitly on a certain occasion such as the case where a remote host previously had no HTTP/2 support gets software upgrade and is now HTTP/2 capable.
trustin
force-pushed
the
h2c_upgrade_efficiency
branch
2 times, most recently
from
ac6b49c
to
89ad8a0
Compare
|
@anuraaga Updated the PR with StampedLock. It's my first time using it, so please review carefully. |
trustin
force-pushed
the
h2c_upgrade_efficiency
branch
from
89ad8a0
to
93ade69
Compare
| } | ||
|
|
||
| final long writeStamp = lock.tryConvertToWriteLock(stamp); | ||
| if (writeStamp == 0L) { |
There was a problem hiding this comment.
Hmm I wonder what use case there is for the SDK not doing this for us... Anyways, can you define a private method convertToWriteLock to share the code?
Motivation:
The current HTTP/1-to-2 upgrade has the following slightly inter-related
issues:
1. When a user specifies an explicit session protocol (H1C/H1/H2C/H2)
instead of HTTP/HTTPS, the session creation must fail when the
negotiated protocol does not match exactly. For example, when a user
specified H2C, a rejected upgrade request should fail session creation.
2. When connecting to a host whose name is registered in /etc/hosts, the
'Host' header of the HEAD upgrade request is not set to the host name
but the host address.
3. When a user specifies HTTP/HTTPS as session protocol and the remote
host does not support H2C/H2, the client currently keeps sending the
HEAD upgrade request. We could cache the list of the hosts with no
HTTP/2 support, so we do not send upgrade requests unnecessarily.
4. Some servers send 'Connection: close' header when rejecting a upgrade
request, resulting disconnection. When this happens and a user specified
HTTP as session protocol, the client should retry the connection attempt
silently, so that a user does not notice the upgrade failure, because
it's not really a failure.
Modifications:
- (2) Fork DefaultHostsFileEntryResolver and HostsFileParser so that
they do not omit the host name in the resolved InetAddress, where
the host name is picked up from by Armeria
- (1) Add SessionProtocolNegotiationException
- Mark the session creation as failure when protocol upgrade fails or
the protocol is different from what user expected
- See HttpConfigurator and HttpSessionChannelFactory
- (3) Add SessionProtocolNegotiationCache
- Update the cache when protocol upgrade is rejected by remote host in
HttpConfigurator
- (4) HttpSessionChannelFactory now retries the connection attempt when
HttpConfigurator finds the upgrade response contains 'Connection:
close' header.
- Add the test cases for (1, 3, 4) to ThriftOverHttpClientTServletIntegrationTest
- Miscellaneous:
- When HTTP codec fails to decode a response, use the cause of the
decoder failure as the invocation result, which is much more
informative.
- Add another variant of Exceptions.logIfUnexpected()
- Add Exceptions.clearTrace() to remove the stack trace of an
exception easily
- Fix a bug where HttpServerHandler sends content in a response to a
HEAD request. A response content of a HEAD request must be empty.
- Upgrade jetty-alpn-agent to 1.0.1.Final to support JDK 8u71/72
Result:
- Invocation now fails when the negotiated protocol does not match the
desired protocol.
- The 'Host' header in a upgrade request now contains host name even if
the host address was resolved via the /etc/hosts file
- HTTP/1-to-2 upgrade request is sent only when necessary.
- Http/1-to-2 upgrade deals better with 'Connection: close' headers.
trustin
force-pushed
the
h2c_upgrade_efficiency
branch
from
93ade69
to
b807c04
Compare
|
LGTM |
|
LGTM2 👍 |
trustin
added a commit
that referenced
this pull request
Feb 16, 2016
More efficient and correct H1C-to-H2C upgrade
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
The current HTTP/1-to-2 upgrade has the following slightly inter-related
issues:
instead of HTTP/HTTPS, the session creation must fail when the
negotiated protocol does not match exactly. For example, when a user
specified H2C, a rejected upgrade request should fail session creation.
'Host' header of the HEAD upgrade request is not set to the host name
but the host address.
host does not support H2C/H2, the client currently keeps sending the
HEAD upgrade request. We could cache the list of the hosts with no
HTTP/2 support, so we do not send upgrade requests unnecessarily.
request, resulting disconnection. When this happens and a user specified
HTTP as session protocol, the client should retry the connection attempt
silently, so that a user does not notice the upgrade failure, because
it's not really a failure.
Modifications:
they do not omit the host name in the resolved InetAddress, where
the host name is picked up from by Armeria
the protocol is different from what user expected
HttpConfigurator
HttpConfigurator finds the upgrade response contains 'Connection:
close' header.
decoder failure as the invocation result, which is much more
informative.
exception easily
HEAD request. A response content of a HEAD request must be empty.
Result:
desired protocol.
the host address was resolved via the /etc/hosts file