markt-asf commented on code in PR #784:
URL: https://github.com/apache/tomcat/pull/784#discussion_r1853716196
##########
java/org/apache/coyote/http2/Stream.java:
##########
@@ -604,10 +611,23 @@ final void writeAck() throws IOException {
final void writeEarlyHints() throws IOException {
MimeHeaders headers = coyoteResponse.getMimeHeaders();
String originalStatus = headers.getHeader(":status");
- headers.setValue(":status").setString("103");
+
headers.setValue(":status").setString(String.valueOf(HttpServletResponse.SC_EARLY_HINTS));
+
try {
- handler.writeHeaders(this, headers, false,
Constants.DEFAULT_HEADERS_FRAME_SIZE);
+ MimeHeaders earlyHintsHeaders = new MimeHeaders();
+ earlyHintsHeaders.duplicate(headers);
+ earlyHintsHeaders.filter(HTTP_EARLY_HINTS_HEADERS);
Review Comment:
This isn't how early hints are meant to work. The headers sent via early
hints are expected to be in the final response.
##########
java/org/apache/coyote/http2/Stream.java:
##########
@@ -604,10 +611,23 @@ final void writeAck() throws IOException {
final void writeEarlyHints() throws IOException {
MimeHeaders headers = coyoteResponse.getMimeHeaders();
String originalStatus = headers.getHeader(":status");
- headers.setValue(":status").setString("103");
+
headers.setValue(":status").setString(String.valueOf(HttpServletResponse.SC_EARLY_HINTS));
Review Comment:
I am in favour of replacing 103 with the correct constant but that is this
only part of the patch that I currently think should be retained.
`Integer.toString()` would be better here though. There are also other places
where 103 should be replaced.
##########
java/org/apache/coyote/http2/Stream.java:
##########
@@ -604,10 +611,23 @@ final void writeAck() throws IOException {
final void writeEarlyHints() throws IOException {
MimeHeaders headers = coyoteResponse.getMimeHeaders();
String originalStatus = headers.getHeader(":status");
- headers.setValue(":status").setString("103");
+
headers.setValue(":status").setString(String.valueOf(HttpServletResponse.SC_EARLY_HINTS));
+
try {
- handler.writeHeaders(this, headers, false,
Constants.DEFAULT_HEADERS_FRAME_SIZE);
+ MimeHeaders earlyHintsHeaders = new MimeHeaders();
+ earlyHintsHeaders.duplicate(headers);
+ earlyHintsHeaders.filter(HTTP_EARLY_HINTS_HEADERS);
+
earlyHintsHeaders.setValue(":status").setString(String.valueOf(HttpServletResponse.SC_EARLY_HINTS));
+ handler.writeHeaders(this, earlyHintsHeaders, false,
Constants.DEFAULT_HEADERS_FRAME_SIZE);
+ // to take advantage of the Early Hints feature, the
server-think-time is needed between the Early Hints
+ // headers and the final response. Therefore, we need emit early
hints status and headers via flush.
+ flush(false);
} finally {
+ // Once early hints emitted, clean up early hints relative headers.
+ // The final response is responsible for resetting Link/CSP/etc
related headers
+ for(String header:HTTP_EARLY_HINTS_HEADERS) {
+ headers.removeHeader(header);
+ }
Review Comment:
These should not be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
