-
Notifications
You must be signed in to change notification settings - Fork 45
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
fix GzipHandler issues with HttpConnector mode enabled on Jetty 9.4 #308
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
Could you explain more on this? I see that the logic in the Jetty12 has the same ordering of handlers as the current order in Jetty9 so how would the ordering not be an issue there? Please explain on what the additional wrapping mentioned does for Jetty 12 which is not done for Jetty9? In case the ordering there doesn't matter, I feel having the same ordering of handlers can be maintained between the Jetty9 and Jetty12 code for uniformity and readability. Otherwise future readers might get confused. |
...mpl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyServletEngineAdapter.java
Show resolved
Hide resolved
// If it has a HttpHeader it is one of the standard headers so won't match any appengine specific header. | ||
if (field.getHeader() != null) { | ||
fields.add(field); | ||
continue; | ||
} |
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 even without this logic, a non-AE header should have been added by the line below no? Am I missing something here since the
switch
is only to certain certain flags and not to prevent addition of fields? - If my above statement is wrong and that we still need to have this check, can we please this condition along with that line mentioned there? Just to have a more consistent flow of readability on what all fields are getting added?
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.
That is correct, it is still correct without this.
This is just an optimization so that we can skip the rest of the logic in the loop as we know straight away it will not match any of these headers.
The Jetty 9.4 handlers have the signature The In Jetty 12 the headers are modified by wrapping the request and overriding |
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.
@maigovannon will do the integration process after final review
for the Jetty 9.4 path the
GzipHandler
is modifying headers of the base Request.However this is occurring after the
JettyRequestAPIData
has already copied the headers and filtered out GAE specific headers from what the application receives, so any modifications to the Request headers byGzipHandler
were not seen by the application.The fix is to insert the
GzipHandler
before theJettyHttpHandler
in the handler chain.This is not an issue with the Jetty 12 path as both the
GzipHandler
and theJettyHttpHandler
wrap the request to modify the headers.I added
GzipHandlerTest
to test this for jetty94, ee8 and ee10 code paths.