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

Sporadic NPE in ArrayByteBufferPool.evict() #11098

Closed
aleksabl opened this issue Dec 30, 2023 · 6 comments · Fixed by #11204
Closed

Sporadic NPE in ArrayByteBufferPool.evict() #11098

aleksabl opened this issue Dec 30, 2023 · 6 comments · Fixed by #11204
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@aleksabl
Copy link

Jetty version(s)
12.0.5

Jetty Environment
ee10

Java version/vendor (use: java -version)
openjdk 21.0.1 2023-10-17 LTS
OpenJDK Runtime Environment Temurin-21.0.1+12 (build 21.0.1+12-LTS)
OpenJDK 64-Bit Server VM Temurin-21.0.1+12 (build 21.0.1+12-LTS, mixed mode, sharing)

OS type/version
Ubuntu 22.04

Description
NullPointerException occurs in ArrayByteBufferPool.evict(). This exception is thrown hundreds of times within approximately 15 seconds. It has occurred twice in our production environment but has not been reproducible in any other environments.

java.lang.NullPointerException: Cannot invoke "org.eclipse.jetty.io.RetainableByteBuffer.capacity()" because "buffer" is null
        at org.eclipse.jetty.io.ArrayByteBufferPool.evict(ArrayByteBufferPool.java:415)
        at org.eclipse.jetty.io.ArrayByteBufferPool.releaseExcessMemory(ArrayByteBufferPool.java:383)
        at org.eclipse.jetty.io.ArrayByteBufferPool.acquire(ArrayByteBufferPool.java:215)
        at org.eclipse.jetty.ee10.servlet.HttpOutput.acquireBuffer(HttpOutput.java:587)
        at org.eclipse.jetty.ee10.servlet.HttpOutput.write(HttpOutput.java:757)

How to reproduce?

@aleksabl aleksabl added the Bug For general bugs on Jetty side label Dec 30, 2023
@sbordet
Copy link
Contributor

sbordet commented Jan 1, 2024

Can you share how did you configure the ArrayByteBufferPool?

@aleksabl
Copy link
Author

aleksabl commented Jan 1, 2024

We're not touching that class in our code. I checked with the debugger, and I see that it is it initiated by its empty constructor from the Server constructor: _bufferPool = bufferPool != null ? bufferPool : new ArrayByteBufferPool();

@sbordet
Copy link
Contributor

sbordet commented Jan 1, 2024

@lorban I think the problem is as follows.

The eviction happens when QueuedPool has at least an idle entry (released concurrently back into the pool by another thread), and it happens to be the oldest.

In this case, calling QueuedEntry.remove() also nulls-out the pooled field, and ArrayByteBufferPool.evict() has logic where remove() is called before getPooled() which therefore returns null, leading to the NPE reported in this issue.

This is a rare case because it needs to happen that there is at least one QueuedEntry (meaning all the ConcurrentEntrys were acquired at one point), and it needs to be released and idle in the QueuedPool, and the acquire() must find the pool in a state where the max memory is exceeded, so eviction triggers.

@sbordet sbordet moved this to 🏗 In progress in Jetty 12.0.6 FROZEN Jan 1, 2024
sbordet added a commit that referenced this issue Jan 1, 2024
Reorganized ArrayByteBufferPool.evict() code to avoid NPE.
Small change to ConcurrentPool: no need to null check the final ConcurrentEntry.holder field.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet linked a pull request Jan 1, 2024 that will close this issue
@sbordet sbordet self-assigned this Jan 1, 2024
@sbordet
Copy link
Contributor

sbordet commented Jan 1, 2024

@aleksabl can you try the code in #11204?

@aleksabl
Copy link
Author

aleksabl commented Jan 3, 2024

@sbordet I'm sorry, but I find it a bit too risky to test it in our production environment (which is the only place this happens)

sbordet added a commit that referenced this issue Jan 3, 2024
* Reorganized ArrayByteBufferPool.evict() code to avoid NPE.
* Fixed acquire logic to take into account that a reserved entry may fail to be enabled because it may be concurrently removed.
* Small change to ConcurrentPool: no need to null check the final ConcurrentEntry.holder field.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Contributor

sbordet commented Jan 3, 2024

Fixed by #11204.

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
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants