fix: Make cached GVR thread-safe #980
Conversation
…ultiple threads are involved
| var cachedGVR = cachedGVRRef.get(); | ||
| if (cachedGVR == null) { | ||
| LOG.debug("No cached GVR found, fetching from database"); | ||
| synchronized (lockForCachedGVR) { |
There was a problem hiding this comment.
Can one of the AtomicReference atomic operations be used to update the value to avoid the synchronized block?
| @@ -36,7 +37,8 @@ public class GenesisValidatorRootValidator { | |||
| private final MetadataDao metadataDao; | |||
| private final FailsafeExecutor<Object> failsafeExecutor; | |||
|
|
|||
| private Bytes32 cachedGenesisValidatorRoot; | |||
| private final AtomicReference<Bytes32> cachedGVRRef = new AtomicReference<>(); | |||
There was a problem hiding this comment.
Looks like we are using our cache, think it would be better to use the Guava cache rather than writing our thread safe implementation
There was a problem hiding this comment.
Hmm, correct. Guava Cache would achieve the same result.
| latch.await(); // Wait for all threads to finish | ||
| executorService.shutdown(); // Shutdown the executor service | ||
|
|
||
| assertThat(allTrue.get()).isTrue(); |
There was a problem hiding this comment.
Don't think this check or the latch is doing anything.
When this fails, it's due to the number of times findGenesisValidatorsRoot is called
There was a problem hiding this comment.
latch.await(); to make sure that all threads have joined. Agreed with allTrue. I added findByGVR assertions later on :). Will be removing allTrue assertion.
There was a problem hiding this comment.
decided to rename allTrue and still kept it.
| @@ -36,7 +37,8 @@ public class GenesisValidatorRootValidator { | |||
| private final MetadataDao metadataDao; | |||
| private final FailsafeExecutor<Object> failsafeExecutor; | |||
|
|
|||
| private Bytes32 cachedGenesisValidatorRoot; | |||
| private final AtomicReference<Bytes32> cachedGVRRef = new AtomicReference<>(); | |||
.../tech/pegasys/web3signer/slashingprotection/validator/GenesisValidatorRootValidatorTest.java
Outdated
Show resolved
Hide resolved
PR Description
Make cached GVR calculation thread-safe. During Web3Signer first boot, if multiple requests for signing comes in, it can result in multiple threads attempting to insert gvr in a fresh database. This would show an constraint violation in database logs.
Fixed Issue(s)
Fixes #978
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Testing