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

Jump to bottom

Each HTTP request unnecessarily starts a new thread, resource leak. #5588

Closed
snazy opened this issue Apr 2, 2024 · 6 comments · Fixed by #5611
Closed

Each HTTP request unnecessarily starts a new thread, resource leak. #5588

snazy opened this issue Apr 2, 2024 · 6 comments · Fixed by #5611

Comments

@snazy
Copy link

snazy commented Apr 2, 2024

Each instance of org.glassfish.jersey.jetty.JettyHttpContainer.ResponseWriter implicitly starts a new thread, because each request creates a new instance of java.util.Timer - and these timer threads do not stop. A big request rate can easily start thousands of timer threads.

From the javadoc of java.util.Timer:

Implementation note: All constructors start a timer thread.

and:

After the last live reference to a Timer object goes away and all outstanding tasks have completed execution, the timer's task execution thread terminates gracefully (and becomes subject to garbage collection). However, this can take arbitrarily long to occur.

So I think that creating a new j.u.Timer instance for every request is not a great solution. A better option would be to have only one timer per container.
@snazy
Copy link
Author

snazy commented Apr 2, 2024

j.u.Timer was introduced by #5372

/cc @zUniQueX

@snazy
Copy link
Author

snazy commented Apr 2, 2024

Or better: use a shared ScheduledThreadPoolExecutor instead of Timer

snazy added a commit to snazy/jersey that referenced this issue Apr 2, 2024
@snazy
Fix thread leak/invasion
4eac685 
Jersey/Jetty, at least in the 3.1 version line, creates one thread for each HTTP request. This behavior was introduced with eclipse-ee4j#5372 and seems not present in the 2.x or 3.x versions of Jersey.

From the javadoc of `java.util.Timer`:
```
Implementation note: All constructors start a timer thread.
...
After the last live reference to a Timer object goes away and all outstanding tasks have completed execution, the timer's task execution thread terminates gracefully (and becomes subject to garbage collection). However, this can take arbitrarily long to occur.
```
It is fair to assume that "arbitrarily long" may also mean _never_, in case GC never runs.

This change replaces the timer & thread per request with a `ScheduledExecutorService` instance per `JettyHttpContainer`.

Also changed the set-timeout mechanism to use `System.nanoTime()` instead of `System.currentTimeMillis()`, because the latter is prone to wall-clock drift and can result into wrong timeout values.

Fixes eclipse-ee4j#5588

Signed-off-by: Robert Stupp <snazy@snazy.de>
snazy added a commit to snazy/jersey that referenced this issue Apr 2, 2024
@snazy
Fix thread leak/invasion
2f20105 
Jersey/Jetty, at least in the 3.1 version line, creates one thread for each HTTP request. This behavior was introduced with eclipse-ee4j#5372 and seems not present in the 2.x or 3.x versions of Jersey.

From the javadoc of `java.util.Timer`:
```
Implementation note: All constructors start a timer thread.
...
After the last live reference to a Timer object goes away and all outstanding tasks have completed execution, the timer's task execution thread terminates gracefully (and becomes subject to garbage collection). However, this can take arbitrarily long to occur.
```
It is fair to assume that "arbitrarily long" may also mean _never_, in case GC never runs.

This change replaces the timer & thread per request with a `ScheduledExecutorService` instance per `JettyHttpContainer`.

Also changed the set-timeout mechanism to use `System.nanoTime()` instead of `System.currentTimeMillis()`, because the latter is prone to wall-clock drift and can result into wrong timeout values.

Fixes eclipse-ee4j#5588

Signed-off-by: Robert Stupp <snazy@snazy.de>
@snazy snazy mentioned this issue Apr 2, 2024
Merged
jansupol pushed a commit that referenced this issue Apr 4, 2024
@snazy @jansupol
Fix thread leak/invasion
de52713 
Jersey/Jetty, at least in the 3.1 version line, creates one thread for each HTTP request. This behavior was introduced with #5372 and seems not present in the 2.x or 3.x versions of Jersey.

From the javadoc of `java.util.Timer`:
```
Implementation note: All constructors start a timer thread.
...
After the last live reference to a Timer object goes away and all outstanding tasks have completed execution, the timer's task execution thread terminates gracefully (and becomes subject to garbage collection). However, this can take arbitrarily long to occur.
```
It is fair to assume that "arbitrarily long" may also mean _never_, in case GC never runs.

This change replaces the timer & thread per request with a `ScheduledExecutorService` instance per `JettyHttpContainer`.

Also changed the set-timeout mechanism to use `System.nanoTime()` instead of `System.currentTimeMillis()`, because the latter is prone to wall-clock drift and can result into wrong timeout values.

Fixes #5588

Signed-off-by: Robert Stupp <snazy@snazy.de>
@zUniQueX
Copy link
Contributor

zUniQueX commented Apr 6, 2024 •
edited
Loading

@snazy Good catch. But I think your current implementation is also missing two aspects here:

  • If Jetty's QTP utilizes all available processors for request handling and timeouts are frequent in some situation, the hard limit of the threads to a quarter of the available processors could lead to much higher timeouts. The delay in the ScheduledExecutorService is the minimum delay of the executor to wait before task execution, so the 'real' delay can be much higher. This wasn't a problem with the Timer implementation because there the timeouts were all handled just when the timeout occurred. I think the executor's pool size should be adjusted relative to Jetty's QTP size.
  • If a request finishes in time, but some other requests are currently in timeout handling, the timeout task of the finished request will be kept in the executor's queue. The Jetty Scheduler implementation calls setRemoveOnCancelPolicy(true) to overcome this problem.

Having that said, I'm thinking, if it would be a good idea to utilize Jetty's Scheduler for the timeout handling. A Scheduler instance can be obtained via request.getComponents().getScheduler().

@senivam
Copy link
Contributor

senivam commented Apr 16, 2024

@snazy, @zUniQueX what do you think about the #5611?

@zUniQueX
Copy link
Contributor

@senivam Looks good to me 👍

@senivam senivam linked a pull request Apr 17, 2024 that will close this issue
Jetty HTTP Container native timeout #5611
Merged
@jansupol
Copy link
Contributor

jansupol commented Apr 25, 2024 •
edited
Loading

Closing as completed via #5611

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Jetty HTTP Container native timeout senivam/jersey
4 participants
@snazy @jansupol @senivam @zUniQueX