-
Notifications
You must be signed in to change notification settings - Fork 80
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
chore: Update to Jetty 12 #6037
base: main
Are you sure you want to change the base?
Conversation
Notes for reviewers:
|
1df0f65
to
0e48ea1
Compare
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.
Files in the py dir LGTM
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.
Looks mostly straightforward; I did not review any of the "new" -11 projects, I'm assuming they are simple moves of the existing projects.
I see these warnings when starting up the server:
2024-09-10T18:33:22.724Z | qtp909786389-131 | WARN | o.e.j.e.s.DefaultServlet | DefaultServlet pathInfoOnly is set to true. Use ResourceServlet instead.
2024-09-10T18:33:22.724Z | qtp909786389-131 | WARN | o.e.j.e.s.ResourceServlet | Deprecated resourceBase used instead of baseResource
2024-09-10T18:33:22.726Z | qtp909786389-131 | WARN | o.e.j.e.s.DefaultServlet | Incorrect mapping for DefaultServlet at /js-plugins/*. Use ResourceServlet
I think we'll want to explicitly add the -11 projects as build targets in publish-ci.yml; I'm not sure whether we'll actually want to publish a native application tar for 11? I think DHE only consumes the JARs from maven central, so does not care about a native application artifact.
@Override | ||
public Instant lastModified() { | ||
// Always return -1, so that we don't get the build system timestamp. In theory, we could return the app startup |
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.
It looks like Instant.ofEpochMilli(-1)
is still treated as a special (as was -1 in Jetty 11), even though it's not documented in the javadoc, at least as used by org.eclipse.jetty.server.ResourceService
:
long ifmsl = HttpDateTime.parseToEpoch(ifms);
if (ifmsl != -1)
{
long lm = content.getResource().lastModified().toEpochMilli();
if (lm != -1 && lm / 1000 <= ifmsl / 1000)
{
writeHttpError(request, response, callback, HttpStatus.NOT_MODIFIED_304);
return true;
}
}
I'm not sure if we actually depend on this behavior, or we just want an old timestamp.
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.
We don't explicitly depend on this behavior, but we want to ensure that we control the caching through something other than "how old is the file". The last-modified value is stored by the client, and then the server (as you quoted) will send the file contents if the file has changed since that date.
Picking a constant is no good then, since that constant will always be the same, the server will never send a new version. Reading the timestamp from the file on disk is an option (and is the default behavior), but this bites us in another way: a release series might look like
- 1/1/2024 release 3.1
- 1/15/2024 release 4.0
- 2/1/2024 release 3.2
If you update from 3.1 to 3.2 and then to 4.0, the files get older, so we fail this check.
As such, -1 to skip sending the last modified at all is probably the best bet. Other options we considered at the time: record the server start time, or server deployment time (somehow), but -1 still seems safer.
Correct - the commits in this branch are separated to allow for easier review, the first is "move everything to jetty 11 projects and packages", the second adds a jetty 12 impl (as "server-jetty" etc), and so on.
Good catch, I'll see if I can resolve this quickly.
Do we get download stats on those GH artifacts? If so, maybe we leave them for a release or two then phase them out, potentially sooner than we might phase out the DHE dependency on the jars from maven central. |
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
-11 projects as build targets in publish-ci.yml
is still outstanding.
Devin discovered that Jetty 12 is noisier when connections are stopped abruptly. We have two use cases that give us three (and a half) stack traces to investigate: Connecting using a web browser to an SSL enabled server, then closing the page produces a pair of exceptions (multiple times, only considering the first pair here, as one error might cause others):
The first exception comes from ExportedTableUpdateListener.onCompleted trying to log a "ExportedTableUpdateListener(5fd9c9e7) is complete" message while the log stream is shutting down. The TableService/ExportedTableUpdates stream is canceled from the client, and is handled by the grpc servlet implementation of jakarta.servlet.AsyncListener.onError. That is propagated to the onCompleted mentioned above, which tries to log a message, which then tries to be written to the ConsoleService/SubscribeToLogs stream. However, that stream apparently wasn't notified - ConsoleServiceGrpcImpl.LogsClient.onCancel() is never invoked. The second in this pair may plausibly be an error in the gRPC servlet implementation, which continues to attempt to write in some error cases, even after it knows that the write has failed. With that said, the contract for jakarta.servlet.ServletResponse.isCommitted is
which doesn't seem to allow for throwing instead of returning "true". In fact, the Jetty implementation specifically returns true in some error states, from org.eclipse.jetty.server.Response:
which seems like it could be reasonable to expect in other error states as well? This third case can be repro'd with either http or https:
or
I'm treating these as only one and a half stack traces, but this does serve to point out that the issue we're experiencing isn't specific to ssl. The client here makes a call to SessionService/CloseSession - when the server handles that, it expires the entire session which synchronously closes the other active gRPC calls with a CANCELED status - for example, the ApplicationService/ListFields call gets a "subscription cancelled" message. However, the ListFields call was already aborted from the jetty side, but we don't seem to get a notification. Tracing the same path in Jetty 12, the GrpcReadListener.onError method was called as the client canceled the call on shutdown, so this may be a regression in Jetty 12. I'll break this out into its own example project and file with Jetty. Note that we somewhat anticipated this issue in our GrpcWebFilter implementation, which in GrpcWebServletRequest catches exceptions from complete() and logs them rather than rethrowing them. Lines 62 to 80 in ff5478d
I think the root cause on both cases is the same - Jetty doesn't seem to be notifying the client of failures on all streams - perhaps only one stream per transport gets the notification? I'll try to reproduce and file this so we can continue. |
Filed jetty/jetty.project#12313 to track this with Jetty - at this time I believe all three stack traces result from this. I also believe that we likely could ignore this issue without negative impacts from the client's perspective, but leaving streams open longer than necessary (until the server next tries to write) will consume unnecessary resources, so we should hold off on updating to Jetty 12 until we have this resolved. |
Great bug report; thanks. |
d45a99f
to
cfd042e
Compare
Need to nail down what is causing that and see about a fix both for jetty 11 (approx vers 11.0.24) and 12. |
Jetty 10/11 are expected to reach End of Life next year, this updates our default server implementation to Jetty 12, and in turn requires Java 17+ at runtime.
Keeps the old Jetty 11 artifact in a new module, release notes will clarify how to keep Jetty 11 (and Java 11) support.
Fixes #5264