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

Consider removing VirtualField.computeIfNull() #4268

Closed
trask opened this issue Oct 2, 2021 · 2 comments · Fixed by #4354
Closed

Consider removing VirtualField.computeIfNull() #4268

trask opened this issue Oct 2, 2021 · 2 comments · Fixed by #4354
Assignees
Labels
enhancement New feature or request

Comments

@trask
Copy link
Member

trask commented Oct 2, 2021

Carrying PR discussion forward from #4262 (comment).

The atomicity guarantee for VirtualField.computeIfNull requires a lock, which has led to problems (#4188).

Just dropping the atomicity guarantee is an option, but it could be confusing to have a method that looks like it provides atomicity signature-wise, but really doesn't (#4192 (comment)).

And if it doesn't provide atomicity anyways, usage of computeIfNull can be replaced by calls to get and set.

@trask trask added the enhancement New feature or request label Oct 2, 2021
@mateuszrzeszutek mateuszrzeszutek self-assigned this Oct 6, 2021
@mateuszrzeszutek
Copy link
Member

I've started working on that and removed almost all usages of computeIfNull - the only one left is ExecutorAdviceHelper#attachContextToTask() - I think it may actually be needed here; it ensures (atomically) that a PropagatedContext is always present, and the PropagatedContext class actually manages the state/context.

I think we can remove the PropagatedContext class together with the computeIfNull method if we simply provide a compareAndSet method on the VirtualField (using an AtomicReferenceFieldUpdater in the generated implementation)

@mateuszrzeszutek
Copy link
Member

The compareAndSet method is another reason for implementing #4241 - since implementing it with a Cache would be quite difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants