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

Mark variables as volatile for safe concurrent access #28

Merged
merged 1 commit into from Apr 10, 2024

Conversation

pjfanning
Copy link
Member

If this change is ok I can port it to jackson-jaxrs-providers.

Shared variables that have mutable values are best marked as volatile. The _defaultAnnotationsToUse variable is only set in the constructors so it feels best to mark it as final to highlight that the value doesn't change and does not need to be volatile.

@cowtowncoder
Copy link
Member

Sounds reasonable; access is messy and I don't really remember original reasoning. But agreed that it is safer to mark as volatile.

The only open thing is that one comment, suggesting PR was intended for 2.17 branch (or needs to updated). Either way is with me, just LMK.

@pjfanning
Copy link
Member Author

This is a follow up to #26 and with that PR brining in optimistic locking behaviour, it becomes more important to have these changes.

@cowtowncoder
Copy link
Member

@pjfanning I understand but my question was specifically if:

  1. This should be against 2.18 branch (if so, comment should be updated to reflect this)
  2. This should be against 2.17 branch (comment ok, PR needs to be rebased or recreated)

so I'm fine merging it but it seems to be in inconsistent state. I could solve it either way but not sure which of above is the intent.

Hope this makes more sense.

@pjfanning
Copy link
Member Author

I'd prefer this to be in 2.17.1. If we choose to delay this to 2.18 then I would prefer to revert #26 and delay it to 2.18 too.

@cowtowncoder
Copy link
Member

Ok. I suppose I should then merge this & cherry-pick, least amount of work.

@cowtowncoder cowtowncoder merged commit 94de8de into FasterXML:2.18 Apr 10, 2024
3 checks passed
@cowtowncoder cowtowncoder changed the title mark variables as volatile Mark variables as volatile for safe concurrent access Apr 10, 2024
cowtowncoder pushed a commit that referenced this pull request Apr 10, 2024
cowtowncoder added a commit that referenced this pull request Apr 10, 2024
@cowtowncoder
Copy link
Member

Done.

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.

None yet

2 participants