Replace synchronization in Zone with locks #306
Conversation
| } | ||
|
|
||
| @Test | ||
| void testReadsWaitForWrites() throws Exception { |
There was a problem hiding this comment.
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.
@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.
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.
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.
So are you okay with me skipping the tests for this file?
There was a problem hiding this comment.
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.
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.
|
|
@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
synchronizedkeyword inZone.javawith 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 thesynchronizedkeyword, 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.