Skip to content

GEODE-6011: Prevent synchronization using strings and boolean#2807

Merged
jake-at-work merged 4 commits intoapache:developfrom
nabarunnag:feature/GEODE-6011
Jan 5, 2019
Merged

GEODE-6011: Prevent synchronization using strings and boolean#2807
jake-at-work merged 4 commits intoapache:developfrom
nabarunnag:feature/GEODE-6011

Conversation

@nabarunnag
Copy link
Copy Markdown
Contributor

* Locking is now achieved using reentrant locks as they are more fair than synchronization.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

@jake-at-work
Copy link
Copy Markdown
Contributor

@nabarunnag any thoughts on using an autoclosable wrapper and using with resource to guard these locks?

@nabarunnag
Copy link
Copy Markdown
Contributor Author

nabarunnag commented Nov 7, 2018

@pivotal-jbarrett Thank you for your suggestion and I felt that it was a good idea to get the lock under a TWR.
But after going through all the articles and Java's Project Coin's decision to not include the locks as TWR, one of which was creating the instance every time something needs to be locked, I am planning to use pure reentrant locks for this PR. Maybe, if needed, we can create a new JIRA to implement this feature in a broader scope of the project rather than just two files.

@jake-at-work
Copy link
Copy Markdown
Contributor

@nabarunnag Good point on the object creation overhead. Been in C++ land so long I didn't even think about that cost of RAII in Java.

	* Locking is now achieved using reentrant locks as they are more fair than synchronization.
Previous locking fix move then locking scope to per-instance from global (per-classloader). This would allow the this function to attempt simultaneous registration of functions and potentially fail for duplicate functions.
private static final long serialVersionUID = 1856043174458190605L;

public static final String ID = "bootstrapping-function";
private static final ReentrantLock registerFuntionLock = new ReentrantLock();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: should be registerFunctionLock

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😄 good catch!

@jake-at-work jake-at-work merged commit fcc61b6 into apache:develop Jan 5, 2019
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