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

In addition to special cases such as avoiding deadlock, make sure that the current thread has got the connectionLock object lock when accessing the statements object #6903

Merged
merged 1 commit into from Jan 27, 2019

Conversation

Projects
None yet
4 participants
@asdf2014
Copy link
Member

asdf2014 commented Jan 23, 2019

  • Use @SuppressWarnings("GuardedBy") instead of noinspection FieldAccessNotGuarded comment

  • Remove @GuardedBy("connectionLock") from connectionLock itself

  • Add FieldAccessNotGuarded into inspection profile and set the level to ERROR

* Use `@SuppressWarnings("GuardedBy")` instead of `noinspection Field…
…AccessNotGuarded` comment

* Remove `@GuardedBy("connectionLock")` from `connectionLock` itself

* Add FieldAccessNotGuarded into inspection profile and set the level to ERROR
@GuardedBy("connectionLock")
private final ConcurrentMap<Integer, DruidStatement> statements;

This comment has been minimized.

@drcrallen

drcrallen Jan 23, 2019

Contributor

There is an explicit exception stated above here, so guarded by is not correct if I'm reading it

This comment has been minimized.

@asdf2014

asdf2014 Jan 24, 2019

Author Member

Hi, @drcrallen . Thanks for your comment. Yep, as the comment says, statements should be synchronized by connectionLock, so we should put a GuardedBy annotation here. And for the explicit exception, we only need to suppress the warning with SuppressWarnings.

This comment has been minimized.

@drcrallen

drcrallen Jan 25, 2019

Contributor

So is the only reason for the ConcurrentMap is so that the remove call does not need to be synchronized?

This comment has been minimized.

@asdf2014

asdf2014 Jan 26, 2019

Author Member

Yes, the original discussion can be found here.

@gianm

gianm approved these changes Jan 27, 2019

Copy link
Contributor

gianm left a comment

LGTM, especially based on the conversation in #6868 (comment).

@gianm gianm merged commit 2b73644 into apache:master Jan 27, 2019

2 checks passed

Inspections: pull requests (Druid) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asdf2014 asdf2014 deleted the asdf2014:noinspection branch Jan 28, 2019

@jon-wei jon-wei added this to the 0.14.0 milestone Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment