-
Notifications
You must be signed in to change notification settings - Fork 233
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
Replace synchronization in Zone with locks #306
base: master
Are you sure you want to change the base?
Replace synchronization in Zone with locks #306
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all lock acquisitions: I'm used to have these outside of the try-finally. Acquisitions may fail, and unlock shouldn't be called then.
} | ||
|
||
@Test | ||
void testReadsWaitForWrites() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this test: T2 waits until T1 has assigned zone
a fully instantiated Zone instance. AFAICT this is equivalent to get rid of the threads and do it sequentially directly in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ibauersachs Do you have a better way to test this functionality? Here's what I'm trying to achieve.
Have a writer thread and a reader thread.
I want to make the reader thread attempt to read the zone
value while the writer thread is writing to it.
In this case, the reader thread should be blocked and then should be able to read the zone only when the writing is complete, and the result should be that the reader thread gets the updated value of zone
. If you have a better idea of doing this, I'm all ears :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the test to not wait for zone
to not become null
. But I still have to add a wait before the reader thread starts up. As I said, feedback/ suggestions are welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is testable without modifying the Zone code to intentionally keep the writeLock until an artificial condition solved. Working with the constructor isn't going to work in any case. You'd need to modify an already created Zone instance from multiple threads.
The current code is still the same as before and equivalent to:
var zone = new Zone(testNameZone, zoneRecordElements);
Name testName = Name.fromConstantString("test.example.");
SetResponse resp = zone.findRecords(testName, Type.ANY);
answers = resp.answers().get(0).size();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So are you okay with me skipping the tests for this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look through all of it. Why did you remove the try/finally blocks? Maybe you misunderstood my comment?
Usually, the pattern for a manual lock looks like this:
void doSomething() {
validation
...
lock.lock();
try {
... do exclusive stuff
}
finally {
lock.unlock();
}
}
} | ||
|
||
@Test | ||
void testReadsWaitForWrites() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is testable without modifying the Zone code to intentionally keep the writeLock until an artificial condition solved. Working with the constructor isn't going to work in any case. You'd need to modify an already created Zone instance from multiple threads.
The current code is still the same as before and equivalent to:
var zone = new Zone(testNameZone, zoneRecordElements);
Name testName = Name.fromConstantString("test.example.");
SetResponse resp = zone.findRecords(testName, Type.ANY);
answers = resp.answers().get(0).size();
But, in that case, if the lock acquisition fails, trying to unlock it in the |
You won't even reach finally if acquisition fails (because it's outside of the try), that's the whole point. |
Ah, got you. Sorry I was reading it wrong. |
SonarCloud Quality Gate failed. 0 Bugs 55.9% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
@ibauersachs any update on this? |
Sorry, I haven't forgotten about your PR. There are some other things I need to take care of before I can come back to it. |
This PR closes #305
It replaces the use of the
synchronized
keyword inZone.java
with reader/ writer locks. The benefit to this is that reader locks will allow multiple threads to simultaneously access the Zones as long as no other thread is writing to it. With thesynchronized
keyword, the reader concurrency is reduced to 1. We are using this library at Comcast for one of our components, and are noticing resource contention while our application is concurrently trying to lookup DNS records for Zones.