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

Memory leak due to DocumentSubsetBitsetCache.keysByIndex #49261

Closed
jpountz opened this issue Nov 18, 2019 · 8 comments · Fixed by #49625 or #50635
Closed

Memory leak due to DocumentSubsetBitsetCache.keysByIndex #49261

jpountz opened this issue Nov 18, 2019 · 8 comments · Fixed by #49625 or #50635
Assignees
@tvernum
Labels
>bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC

Comments

@jpountz
Copy link
Contributor

jpountz commented Nov 18, 2019

While reviewing DocumentSubsetBitsetCache, I noticed that it might leak memory since size-based evictions don't trigger evictions in DocumentSubsetBitsetCache.keysByIndex. So keysByIndex might keep holding a reference to cache keys long after they have been removed from the cache.
@jpountz jpountz added >bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Nov 18, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Authorization)

@tvernum tvernum self-assigned this Nov 19, 2019
jkakavas added a commit to jkakavas/elasticsearch that referenced this issue Nov 27, 2019
@jkakavas
Use PEM files instead of a JKS for key material
eefada0 
So that the tests can also run in a FIPS 140 JVM, where using a
JKS keystore is not allowed.

Resolves: elastic#49261
@jkakavas jkakavas mentioned this issue Nov 27, 2019
Merged
jkakavas added a commit that referenced this issue Nov 29, 2019
@jkakavas
Use PEM files instead of a JKS for key material (#49625)
ec8553f 
So that the tests can also run in a FIPS 140 JVM, where using a
JKS keystore is not allowed.

Resolves: #49261

* resolve name collision
jkakavas added a commit to jkakavas/elasticsearch that referenced this issue Nov 29, 2019
@jkakavas
Use PEM files instead of a JKS for key material (elastic#49625)
24cbad8 
So that the tests can also run in a FIPS 140 JVM, where using a
JKS keystore is not allowed.

Resolves: elastic#49261

* resolve name collision
@jpountz
Copy link
Contributor Author

jpountz commented Nov 29, 2019

@jkakavas Did you close the wrong issue?

jkakavas added a commit that referenced this issue Nov 29, 2019
@jkakavas
Use PEM files instead of a JKS for key material (#49625) (#49701)
a59b7e0 
So that the tests can also run in a FIPS 140 JVM, where using a
JKS keystore is not allowed.

Resolves: #49261
@jkakavas
Copy link
Member

Yeap, wrong resolves link :/

@jkakavas jkakavas reopened this Nov 29, 2019
@olegbonar
Copy link
Contributor

Hi @jpountz! If I understand you correctly we can try using a RemovalListener in order to remove the leaking entry from keysByIndex too. What do you think?

@jpountz
Copy link
Contributor Author

jpountz commented Dec 6, 2019

Yes this is the idea. The tricky part is to make sure there are no race conditions in case a query is removed from the cache and shortly added back afterwards.

@olegbonar
Copy link
Contributor

olegbonar commented Dec 20, 2019 •
edited
Loading

@jpountz, I'm not a concurrency expert but should we make adding to the cache and adding to the hashmap a single atomic operation? Aren't they atomic now?

@tvernum
Copy link
Contributor

tvernum commented Dec 31, 2019

@olegbonar Have you made any progress on this? I'd like to fix this bug in the next couple of weeks, but I'm happy to leave it for you if you've got something underway.

@olegbonar
Copy link
Contributor

@tvernum, I haven't. Please take it.

@tvernum tvernum mentioned this issue Jan 6, 2020
Merged
This was referenced Feb 3, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tvernum tvernum

Labels
>bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC
Projects
None yet
Milestone
No milestone
5 participants
@jpountz @tvernum @jkakavas @elasticmachine @olegbonar