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

sonar :: add synchronized to clear() and isInUse() methods in HDMobil… #426

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

dev-mlb
Copy link
Collaborator

@dev-mlb dev-mlb commented Mar 2, 2023

…eAgent to match parent class implementation

jdcove2
jdcove2 previously approved these changes Mar 3, 2023
Copy link
Collaborator

@jdcove2 jdcove2 left a comment

Choose a reason for hiding this comment

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

Generally, synchronization on instance methods (i.e. "this") and static methods (i.e. "Clazz.class") is discouraged because other threads can also synchronize on "this" and "Clazz.class", which can cause deadlocks. The preferred way is to use synchronization blocks on a private object. I usually create a private object just for this (e.g. "private final Object lock = new Object()").

Given that this method already has a bunch of synchronized instance methods, synchronizing two more hopefully will not be a problem. I would suggest careful full-scale testing of this PR as this is the main multi-threaded entry point.

@dev-mlb
Copy link
Collaborator Author

dev-mlb commented Mar 3, 2023

Generally, synchronization on instance methods (i.e. "this") and static methods (i.e. "Clazz.class") is discouraged because other threads can also synchronize on "this" and "Clazz.class", which can cause deadlocks. The preferred way is to use synchronization blocks on a private object. I usually create a private object just for this (e.g. "private final Object lock = new Object()").

Given that this method already has a bunch of synchronized instance methods, synchronizing two more hopefully will not be a problem. I would suggest careful full-scale testing of this PR as this is the main multi-threaded entry point.

Agreed, but when @Overrides of synchronized methods are not themselves synchronized, the result can be improper synchronization as callers rely on the thread-safety promised by the parent class. I felt like the best option, at the moment, was to mark these as synchronized to match the parent implementation and not refactor all of MobileAgent/HDMobileAgent.

@jpdahlke jpdahlke added this to the 7.16.0 milestone Mar 3, 2023
@jpdahlke jpdahlke added the tech-debt Low-impact cleanup and upkeep label Mar 3, 2023
fbruton
fbruton previously approved these changes Mar 10, 2023
@jpdahlke jpdahlke modified the milestones: v7.16.0, v7.17.0 Mar 25, 2023
@jpdahlke jpdahlke dismissed stale reviews from fbruton and jdcove2 via d474e2d March 28, 2023 13:50
@cfkoehler cfkoehler self-requested a review April 11, 2023 00:01
@jpdahlke jpdahlke merged commit 9cb7a95 into NationalSecurityAgency:master Apr 21, 2023
@dev-mlb dev-mlb deleted the sonar-synchronized branch May 5, 2023 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Low-impact cleanup and upkeep
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants