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

Fixes #10211 - NPE in ArrayByteBufferPool.findOldestEntry() #10212

Merged
merged 3 commits into from
Aug 2, 2023

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Aug 2, 2023

Fixed algorithm to check for oldest entry to avoid NPE. Added comments for clarity.

Fixed algorithm to check for oldest entry to avoid NPE.
Added comments for clarity.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet requested a review from gregw August 2, 2023 14:25
@sbordet sbordet linked an issue Aug 2, 2023 that may be closed by this pull request
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

A couple of niggles, but I won't request changes in case I'm out of contact and can't approve any changes. So I'm -0 as is, and +1 with the niggles fixed.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet requested review from gregw and joakime August 2, 2023 14:56
Comment on lines 445 to 452
if (buffer == null)
continue;
long age = NanoTime.elapsed(buffer.getLastUpdate(), now);
if (oldestBuffer != null && age < oldestAge)
continue;
oldestEntry = entry;
oldestBuffer = buffer;
oldestAge = age;
Copy link
Contributor

Choose a reason for hiding this comment

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

There you go... you can be both consistent and readable:

Suggested change
if (buffer == null)
continue;
long age = NanoTime.elapsed(buffer.getLastUpdate(), now);
if (oldestBuffer != null && age < oldestAge)
continue;
oldestEntry = entry;
oldestBuffer = buffer;
oldestAge = age;
if (buffer != null)
{
long age = NanoTime.elapsed(buffer.getLastUpdate(), now);
if (oldestBuffer == null || age > oldestAge)
{
oldestEntry = entry;
oldestBuffer = buffer;
oldestAge = age;
}
}

gregw
gregw previously approved these changes Aug 2, 2023
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

One niggle left, but I'll approve anyway

@gregw
Copy link
Contributor

gregw commented Aug 2, 2023

@sbordet ... and we should add tests for this... but in another later PR is fine

@@ -390,13 +392,14 @@ private void evict(boolean direct, long excess)

if (oldestEntry.remove())
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@gregw gregw Aug 2, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR changes ArrayRetainableByteBufferPool.java in jetty-10.0.x and is missing that null check.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to retract and get more sleep
Disregard everything I've said in the last hour. :-(

Copy link
Contributor

Choose a reason for hiding this comment

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

Signed-off-by: Simone Bordet <[email protected]>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Thanks

@sbordet sbordet merged commit 30ed83f into jetty-10.0.x Aug 2, 2023
2 checks passed
@sbordet sbordet deleted the fix/jetty-10-10211-npe-findOldestEntry branch August 2, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE in ArrayByteBufferPool.findOldestEntry()
3 participants