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

BEANUTILS-539/BEANUTILS-509: 1.X line, switch WeakFastHashMap for ConcurrentWeakHashMap #95

Open
wants to merge 1 commit into
base: 1.X
Choose a base branch
from

Conversation

car-roll
Copy link

@car-roll car-roll commented Sep 1, 2021

Porting the changes made in #56 to the 1.X line

@garydgregory
Copy link
Member

garydgregory commented Sep 2, 2021

This will break binary compatibility and I don't see releasing out of 1.x anyway as we are resource-constrained and want to get 2.0 out.

A general comment: This PR adds an extremely complex class (ConcurrentWeakKeyHashMap) with ZERO tests to match, a no-go.

Since this branch is more than likely going nowhere unless there is a security issue to remedy, I would not spend this on this branch.

@olamy
Copy link
Member

olamy commented Sep 2, 2021

@garydgregory this issue makes Jenkins totally irresponsive... see https://issues.jenkins.io/browse/JENKINS-60997
such race condition which depends on the garbage collector (because of weakreference) is definitely impossible to reproduce 100% with a unit test.
what could be your proposal alternative to fix that?

@garydgregory
Copy link
Member

@garydgregory this issue makes Jenkins totally irresponsive... see https://issues.jenkins.io/browse/JENKINS-60997
such race condition which depends on the garbage collector (because of weakreference) is definitely impossible to reproduce 100% with a unit test.
what could be your proposal alternative to fix that?

I expect the new classes to be tested thoroughly. I can understand that the whole use case might be hard to reproduce but a whole new class needs a whole new test to match it. Public and protected APIs cannot be changed.

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.

3 participants