Fixes #3766 - Introduce HTTP/2 API to batch frames. #5222
Conversation
Introduced Stream.FrameList to hold HEADERS+DATA+HEADERS frames. These are often used by the client and by the server when the request/response content is known and FrameList will allow to send them in a single TCP write, rather than multiple ones. Rewritten HttpSenderOverHTTP2.sendHeaders() and HttpTransportOverHTTP2.sendHeaders() to take advantage of FrameList. Now using ConcurrentHashMap as a client context, because with DEBUG logging enabled it may be access concurrently. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Fixed HttpTransportOverHTTP2.sendHeaders() - HEADERS frame for trailers was missing the stream id. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
| /** | ||
| * <p>An ordered list of frames belonging to the same stream.</p> | ||
| */ | ||
| public static class FrameList |
There was a problem hiding this comment.
why a class rather than just some utility methods to build List<StreamFrame> ? They could equally check that all frames are the same streamId, but ultimately why check at creation when you must have to check when sent. If a single DataFrame is sent with the wrong streamId, it is the same problem
There was a problem hiding this comment.
I find using a dedicated class more elegant as it is a clear API indicator that a method accepting a FrameList is won't modify the collection of StreamFrames. I'd just explicitly state in the javadoc of this class that immutable.
| */ | ||
| public List<StreamFrame> getFrames() | ||
| { | ||
| return frames; |
There was a problem hiding this comment.
this is mutable. Is it intended to be?
|
|
||
| package org.eclipse.jetty.http2.frames; | ||
|
|
||
| public abstract class StreamFrame extends Frame |
There was a problem hiding this comment.
javadoc please.
Maybe make it an inner class of Frame?
Is there a ConnectionFrame class to match?
|
|
||
| /** | ||
| * <p>Sends the given HEADERS {@code frame}.</p> | ||
| * <p>Typically used to send an HTTP response with no content and no trailers, |
There was a problem hiding this comment.
what about sending request with no body?
| public FrameList(HeadersFrame headers) | ||
| { | ||
| Objects.requireNonNull(headers); | ||
| this.frames = Collections.singletonList(headers); |
There was a problem hiding this comment.
I would store a unmodifiableList in this.frames.
| public FrameList(HeadersFrame headers, DataFrame data, HeadersFrame trailers) | ||
| { | ||
| Objects.requireNonNull(headers); | ||
| this.frames = new ArrayList<>(3); |
There was a problem hiding this comment.
I would store a unmodifiableList in this.frames.
| /** | ||
| * <p>An ordered list of frames belonging to the same stream.</p> | ||
| */ | ||
| public static class FrameList |
There was a problem hiding this comment.
The public static modifiers are not needed.
| /** | ||
| * <p>An ordered list of frames belonging to the same stream.</p> | ||
| */ | ||
| public static class FrameList |
There was a problem hiding this comment.
I find using a dedicated class more elegant as it is a clear API indicator that a method accepting a FrameList is won't modify the collection of StreamFrames. I'd just explicitly state in the javadoc of this class that immutable.
Updates after review. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Introduced Stream.FrameList to hold HEADERS+DATA+HEADERS frames.
These are often used by the client and by the server when the
request/response content is known and FrameList will allow to
send them in a single TCP write, rather than multiple ones.
Rewritten HttpSenderOverHTTP2.sendHeaders() and
HttpTransportOverHTTP2.sendHeaders() to take advantage of
FrameList.
Now using ConcurrentHashMap as a client context, because
with DEBUG logging enabled it may be access concurrently.
Signed-off-by: Simone Bordet simone.bordet@gmail.com