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.

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

HttpConfiguration.setIdleTimeout() breaks long running requests #10229

Closed
ekupcik opened this issue Aug 4, 2023 · 2 comments · Fixed by #10233
Closed

HttpConfiguration.setIdleTimeout() breaks long running requests #10229

ekupcik opened this issue Aug 4, 2023 · 2 comments · Fixed by #10233
Labels
Bug For general bugs on Jetty side

Comments

@ekupcik
Copy link

ekupcik commented Aug 4, 2023

Jetty version(s)
12 beta 4

Java version/vendor (use: java -version)
17

OS type/version
Windows

Description
I am not sure whether this might be intentional or not. But it looks more like a bug to me or at least like an unexpected change in Jetty 12.

From my understanding HttpConfiguration.idleTimeout refers to timeout in IO operations, not long running application code. In my code i was setting idleTimeoutout in Jetty 9 and 10 without any issues but with Jetty 12 long running request fail this error when the application code takes longer than the idleTimeout. Also note the suppressed NPE

HTTP ERROR 500 java.util.concurrent.TimeoutException: Idle timeout expired: 5015/5000 ms

URI: http://localhost:8080/
500
java.util.concurrent.TimeoutException: Idle timeout expired: 5015/5000 ms
java.util.concurrent.TimeoutException: Idle timeout expired: 5015/5000 ms

Caused by:

java.util.concurrent.TimeoutException: Idle timeout expired: 5015/5000 ms
	at org.eclipse.jetty.io.IdleTimeout.checkIdleTimeout(IdleTimeout.java:166)
	at org.eclipse.jetty.io.IdleTimeout.idleCheck(IdleTimeout.java:112)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)
	Suppressed: java.lang.NullPointerException: Cannot invoke "java.util.function.Consumer.accept(Object)" because "onFailure" is null
		at org.eclipse.jetty.server.internal.HttpChannelState.lambda$onFailure$2(HttpChannelState.java:437)
		at org.eclipse.jetty.util.thread.SerializedInvoker$Link.run(SerializedInvoker.java:191)
		at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:969)
		at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1194)
		at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1149)
		... 1 more

How to reproduce?

        public static void main(String... args) throws Exception
	{
		HttpServlet servlet = new HttpServlet()
		{
			@Override
			protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
			{
				try
				{
					Thread.sleep(10000);
				}
				catch (InterruptedException e)
				{
					Thread.currentThread().interrupt();
				}

				resp.getWriter().printf("hello world");
			}
		};

		ServletContextHandler handler = new ServletContextHandler();
		handler.setContextPath("/");
		handler.addServlet(servlet, "/*");

		Server server = new Server();
		server.setHandler(handler);

		HttpConfiguration configuration = new HttpConfiguration();
		configuration.setIdleTimeout(5000);

		ServerConnector connector = new ServerConnector(server, new HttpConnectionFactory(configuration));
		connector.setPort(8080);
		server.setConnectors(new ServerConnector[]{connector});

		server.start();
	}
@ekupcik ekupcik added the Bug For general bugs on Jetty side label Aug 4, 2023
@gregw
Copy link
Contributor

gregw commented Aug 4, 2023

Unintentional change. We know that the servlet spec requires that long running requests are not interrupted by idle timeout.... however, we struggle to not close the connection under them sometimes.
We have the mechanism to ignore idle timeouts, we just need to correctly use it in ee10 servlets. This might not make 12.0.0.... specially as we need to review why our tests for this didn't spot it. But 12.0.1 is only weeks away.

gregw added a commit that referenced this issue Aug 4, 2023
Added test to reproduce
@gregw gregw linked a pull request Aug 4, 2023 that will close this issue
@gregw
Copy link
Contributor

gregw commented Aug 4, 2023

I've created draft PR #10233 that has a unit test that reproduces the problem.

Note that I also do not like the NPE that is suppressed, so there are at least 2 issues to fix.

gregw added a commit that referenced this issue Aug 4, 2023
Fixed NPE if no failure listener
gregw added a commit that referenced this issue Aug 4, 2023
gregw added a commit that referenced this issue Aug 4, 2023
Added test that idle works between requests
gregw added a commit that referenced this issue Aug 5, 2023
EE9 idle timeout
gregw added a commit that referenced this issue Aug 5, 2023
idle if read operation
gregw added a commit that referenced this issue Aug 5, 2023
Handle idleTimeout for IO operations differently
gregw added a commit that referenced this issue Aug 5, 2023
improve comments
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.0 Aug 5, 2023
gregw added a commit that referenced this issue Aug 5, 2023
fixed test to not expect timeout listener to be called if there is demand
gregw added a commit that referenced this issue Aug 5, 2023
Idle timeouts for IO operations are not last.
gregw added a commit that referenced this issue Aug 6, 2023
Disable transient idle timeouts since AsyncContentProducer cannot handle them.
gregw added a commit that referenced this issue Aug 6, 2023
revert test to persistent idle failures
gregw added a commit that referenced this issue Aug 6, 2023
* Fix #10229  Idle Timeout

Added test to reproduce

Fixed NPE if no failure listener


Possible

Added test that idle works between requests

EE9 idle timeout

idle if read operation

Handle idleTimeout for IO operations differently

improve comments

fixed test to not expect timeout listener to be called if there is demand

Idle timeouts for IO operations are not last.

Disable transient idle timeouts since AsyncContentProducer cannot handle them.

revert test to persistent idle failures
@sbordet sbordet moved this from 🏗 In progress to ✅ Done in Jetty 12.0.0 Aug 6, 2023
@sbordet sbordet closed this as completed Aug 6, 2023
gregw added a commit that referenced this issue Aug 10, 2023
Cleanup downcasting
gregw added a commit that referenced this issue Aug 10, 2023
Cleanup downcasting
gregw added a commit that referenced this issue Aug 11, 2023
Protect close after lasts
gregw added a commit that referenced this issue Aug 11, 2023
Prefer the wrapped servlet request/response
gregw added a commit that referenced this issue Aug 11, 2023
Do not use Wrappers for the wrapped servlet request/response
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants