Skip to content
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.

Sign up for GitHub

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

servlet: ignore IllegalStateException from AsyncContext.complete() #11819

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

servlet: ignore IllegalStateException from AsyncContext.complete() #11819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b76d860
ignore IllegalStateException from asyncContext.complete()
panchenko Jan 13, 2025
20feb57
fix checkstyle
panchenko Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,12 @@
transportState.runOnTransportThread(
() -> {
transportState.complete();
asyncContext.complete();
try {
asyncContext.complete();
} catch (IllegalStateException ignored) {
// Tomcat can throw:
// Calling [asyncComplete()] is not valid for a request with Async state [COMPLETING]
Copy link
Contributor

@kannanjgithub kannanjgithub Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried reproing by inducing a timeout and I see that the org.eclipse.jetty.server.HttpChannelState is getting set to EXPIRE

if (this._requestState == HttpChannelState.RequestState.ASYNC) {
          this._requestState = HttpChannelState.RequestState.EXPIRE;

timeout:615, HttpChannelState (org.eclipse.jetty.server)
run:148, AsyncContextEvent (org.eclipse.jetty.server)
call:539, Executors$RunnableAdapter (java.util.concurrent)
run:264, FutureTask (java.util.concurrent)
run:304, ScheduledThreadPoolExecutor$ScheduledFutureTask (java.util.concurrent)
runWorker:1136, ThreadPoolExecutor (java.util.concurrent)
run:635, ThreadPoolExecutor$Worker (java.util.concurrent)
run:833, Thread (java.lang)

and that the exception thrown from asyncContext.complete(); has the message

s=HANDLING rs=EXPIRE os=OPEN is=IDLE awp=false se=false i=false al=1

It did not have the state COMPLETING.

Copy link
Contributor Author

@panchenko panchenko Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The message is different in different servers (Tomcat / Jetty), as state names is an implementation detail.
  • It happens multiple times when running tests for this module, like connection closed by client after deadline exceeded, etc
  • I have tried to add more checks before this call, some exceptions can be avoided with that, but then decided it does not worth it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IllegalStateException means we've either called something incorrectly (so we should fix it) or there's a bug in the servlet container (and we should file a bug). I don't think we should have something like this without a tracking issue to fix whatever the actual bug is, as this should be removed medium-term.

}
log.fine("call completed");
});
};
Expand Down Expand Up @@ -254,7 +259,7 @@
@VisibleForTesting // Lincheck test can not run with java.util.logging dependency.
interface Log {
default boolean isLoggable(Level level) {
return false;
return false;

Check warning on line 262 in servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java

View check run for this annotation

Codecov / codecov/patch

servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java#L262 

Added line #L262 was not covered by tests
}

default void fine(String str, Object...params) {}
Expand Down
7 changes: 6 additions & 1 deletion servlet/src/main/java/io/grpc/servlet/ServletServerStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,12 @@ public void cancel(Status status) {
close(Status.CANCELLED.withCause(status.asRuntimeException()), new Metadata());
CountDownLatch countDownLatch = new CountDownLatch(1);
transportState.runOnTransportThread(() -> {
asyncCtx.complete();
try {
asyncCtx.complete();
} catch (IllegalStateException ignored) {
// Tomcat can throw:
// Calling [asyncComplete()] is not valid for a request with Async state [COMPLETING]
}
countDownLatch.countDown();
});
try {
Expand Down
Loading