Replace synchronization in Zone with locks by srijeet0406 · Pull Request #306 · dnsjava/dnsjava

srijeet0406 · 2023-11-30T00:03:44Z

This PR closes #305

It replaces the use of the synchronized keyword in Zone.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 the synchronized 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.

ibauersachs

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.

src/main/java/org/xbill/DNS/Zone.java

ibauersachs · 2023-12-03T16:33:38Z

src/test/java/org/xbill/DNS/ZoneTest.java

+  }
+
+  @Test
+  void testReadsWaitForWrites() throws Exception {


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.

@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 :)

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!

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();

So are you okay with me skipping the tests for this file?

ibauersachs

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();
  }
}

pom.xml

src/main/java/org/xbill/DNS/Zone.java

ibauersachs · 2023-12-10T15:12:41Z

src/test/java/org/xbill/DNS/ZoneTest.java

+  }
+
+  @Test
+  void testReadsWaitForWrites() throws Exception {


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();

srijeet0406 · 2023-12-11T18:34:17Z

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();
  }
}

But, in that case, if the lock acquisition fails, trying to unlock it in the finally block will throw an exception.

ibauersachs · 2023-12-11T19:29:53Z

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();
  }
}
But, in that case, if the lock acquisition fails, trying to unlock it in the finally block will throw an exception.

You won't even reach finally if acquisition fails (because it's outside of the try), that's the whole point.

srijeet0406 · 2023-12-11T19:31:57Z

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();
  }
}
But, in that case, if the lock acquisition fails, trying to unlock it in the finally block will throw an exception.
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.

sonarqubecloud · 2023-12-11T20:14:25Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

55.9% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

srijeet0406 · 2024-01-29T18:48:10Z

@ibauersachs any update on this?

ibauersachs · 2024-01-30T06:53:31Z

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.

Replace synchronization in Zone with locks

37c70f5

ibauersachs requested changes Dec 3, 2023

View reviewed changes

srijeet0406 added 2 commits December 4, 2023 15:31

code review

9d59395

cleanup

0b0d96e

ibauersachs requested changes Dec 10, 2023

View reviewed changes

srijeet0406 added 2 commits December 11, 2023 13:05

moving unlocking into finally block, other cleanup

bbe8239

remove unused variables

a96aba2

ibauersachs closed this in 2f9b45b Jul 21, 2024

ibauersachs mentioned this pull request Jul 21, 2024

Need for synchronization while looking up Zones #305

Closed

Conversation

srijeet0406 commented Nov 30, 2023

Uh oh!

ibauersachs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ibauersachs Dec 3, 2023

Choose a reason for hiding this comment

Uh oh!

srijeet0406 Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

srijeet0406 Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

ibauersachs Dec 10, 2023

Choose a reason for hiding this comment

Uh oh!

srijeet0406 Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

ibauersachs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ibauersachs Dec 10, 2023

Choose a reason for hiding this comment

Uh oh!

srijeet0406 commented Dec 11, 2023

Uh oh!

ibauersachs commented Dec 11, 2023

Uh oh!

srijeet0406 commented Dec 11, 2023

Uh oh!

sonarqubecloud bot commented Dec 11, 2023

Uh oh!

srijeet0406 commented Jan 29, 2024

Uh oh!

ibauersachs commented Jan 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants