-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
implement servlet upgrade for ee10 and ee11 #12186
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
The TCK fully passes after the changes in this branch. |
YEAHHHH |
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.
I think it should be possible to do this without making HttpConnection public. We already have some upgrade related interfaces, so use them or create a new one to abstract away from the implementation.
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
@@ -11,7 +11,7 @@ | |||
// ======================================================================== | |||
// | |||
|
|||
package org.eclipse.jetty.server.internal; |
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.
Can you explain why this move is necessary?
Is it so that ServletAPIRequest
can do:
HttpConnection httpConnection = (HttpConnection)_servletContextRequest.getConnectionMetaData().getConnection();
httpConnection.getParser().servletUpgrade();
outputStream.flush(); // commit the 101 response
httpConnection.getGenerator().servletUpgrade(); // tell the generator it can send data as-is
We already have interfaces: Connection.UpgradeFrom
, Connection.UpgradeTo
to abstract away from specific connection types. so can't you use them instead? Get those methods to flip the parser and generator states?
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.
Is it so that ServletAPIRequest can do:
Yes, this is why it was necessary.
We already have interfaces: Connection.UpgradeFrom, Connection.UpgradeTo to abstract away from specific connection types. so can't you use them instead? Get those methods to flip the parser and generator states?
These interfaces are already implemented on HttpConnection
, I think for the HTTP/2 upgrade.
So I think the options are (unless you have a better idea)
- make
HttpConnection
public - create a new
Upgrade
interface and implement it on theHttpConnection
- add a new method to the
Connection
interface.
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.
Option 2
…to upgrade() Signed-off-by: Lachlan Roberts <[email protected]>
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.
I'd like to keep HttpConnection internal
…de interface Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
@gregw nudge |
@@ -596,7 +596,7 @@ public ComplianceViolation.Listener getComplianceViolationListener() | |||
* <dl> | |||
* <dt>org.eclipse.jetty.server.Server</dt><dd>The Jetty Server instance</dd> | |||
* <dt>org.eclipse.jetty.server.HttpChannel</dt><dd>The HttpChannel for this request</dd> | |||
* <dt>org.eclipse.jetty.server.HttpConnection</dt><dd>The HttpConnection or null if another transport is used</dd> | |||
* <dt>org.eclipse.jetty.server.internal.HttpConnection</dt><dd>The HttpConnection or null if another transport is used</dd> |
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 is not correct (nor was it before). It is actually looking for Connection.class.getName()
public void upgrade() | ||
{ | ||
getParser().upgrade(); | ||
getGenerator().upgrade(); | ||
} |
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.
Sorry, but I've changed my mind about the naming here. This is not really upgrade
, it would be more accurate to call these methods tunnel
or openTunnel
or startTunnel
.
The description would be to convert the connection over which the current protocol us running to a tunnel, but without replacing the connection (as is done by our real upgrade mechanism). This can be used for upgrade within a connection, but it is not really an upgrade for this connection, as the connection remains and just tunnels data to/from its endpoint.
Signed-off-by: Lachlan Roberts <[email protected]>
Implement Servlet upgrade for ee10 and ee11 which is required for the TCK.
Replaces #10128