-
Notifications
You must be signed in to change notification settings - Fork 4k
Implement missing authority API for HandlerRegistry #2709
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,9 @@ | |
| import io.netty.handler.codec.http2.Http2Stream; | ||
| import io.netty.handler.codec.http2.Http2StreamVisitor; | ||
| import io.netty.handler.logging.LogLevel; | ||
| import io.netty.util.AsciiString; | ||
| import io.netty.util.ReferenceCountUtil; | ||
|
|
||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
| import javax.annotation.Nullable; | ||
|
|
@@ -96,6 +98,7 @@ class NettyServerHandler extends AbstractNettyHandler { | |
| private Throwable connectionError; | ||
| private boolean teWarningLogged; | ||
| private WriteQueue serverWriteQueue; | ||
| private AsciiString lastKnownAuthority; | ||
|
|
||
| static NettyServerHandler newHandler(ServerTransportListener transportListener, | ||
| int maxStreams, | ||
|
|
@@ -190,8 +193,9 @@ private void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers | |
| checkNotNull(transportListener.methodDetermined(method, metadata), "statsTraceCtx"); | ||
| NettyServerStream.TransportState state = new NettyServerStream.TransportState( | ||
| this, http2Stream, maxMessageSize, statsTraceCtx); | ||
| String authority = getOrUpdateAuthority((AsciiString)headers.authority()); | ||
| NettyServerStream stream = new NettyServerStream(ctx.channel(), state, attributes, | ||
| statsTraceCtx); | ||
| authority, statsTraceCtx); | ||
| transportListener.streamCreated(stream, method, metadata); | ||
| state.onStreamAllocated(); | ||
| http2Stream.setProperty(streamKey, state); | ||
|
|
@@ -205,6 +209,18 @@ private void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers | |
| } | ||
| } | ||
|
|
||
| private String getOrUpdateAuthority(AsciiString authority) { | ||
| if (authority == null) { | ||
| return null; | ||
| } else if (!authority.equals(lastKnownAuthority)) { | ||
| lastKnownAuthority = authority; | ||
| } | ||
|
|
||
| // AsciiString.toString() is internally cached, so subsequent calls will not | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not efficient enough. If the client changes the authority every request and the server doesn't read from it, it will unnecessarily convert it. Why not do the conversion when getAuthority is called?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getAuthority() is always called. This is plenty efficient enough. It doesn't hinder the common case at all. Most clients, by far, won't change the authority every request. And even if they do change constantly, then any optimization here would probably be in the noise. I'd rather get to a point we can call good enough for now and any future optimization for edge cases gets included with the HPACK tricks we're planning.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I talked with Carl. He showed me that getAuthority() isn't always called, since it is only called when the immutable lookup fails. But we also agreed that it will 99% of the time always be the same authority. And we also believe that the HPACK table will even make it the same AsciiString instance. So Carl is now okay with this. He would still like to have our nice HPACK stuff, but that's orthogonal and future work. |
||
| // result in recomputing the String representation of lastKnownAuthority. | ||
| return lastKnownAuthority.toString(); | ||
| } | ||
|
|
||
| private void onDataRead(int streamId, ByteBuf data, int padding, boolean endOfStream) | ||
| throws Http2Exception { | ||
| flowControlPing().onDataRead(data.readableBytes(), padding); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not be thread safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InProcessStream.this.authorityis nowvolatile. Setting and reading references are atomic in Java. Strings are immutable. This should be thread safe now, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it was fine as it was before (without volatile). setAuthority "May only be called before start."