Make blocks cache locked in Store when used + minor cleanup #7594
Conversation
zilm13
changed the title
| @@ -37,6 +38,22 @@ static BlockProvider fromDynamicMap(Supplier<Map<Bytes32, SignedBeaconBlock>> ma | |||
| return (roots) -> fromMap(mapSupplier.get()).getBlocks(roots); | |||
| } | |||
|
|
|||
| static BlockProvider fromMapWithLock( | |||
There was a problem hiding this comment.
should probably add a test case
There was a problem hiding this comment.
Thanks you asked for it, I made all stupidly wrong, fixed
There was a problem hiding this comment.
I did the wrong thing again, thinking how to do it correctly
There was a problem hiding this comment.
So my understanding on this is:
- We should remove
SafeFuture.supplyAsync()to avoid such errors, noone was using it, and we should use our own AsyncRunner whenever it's needed - In this case we should use our AsyncRunner
- We should also use it in Store methods that use locks and return SafeFuture otherwise they are synchronously locked (
retrieveFinalizedCheckpointAndState,createStateGenerationTask) - It will require rework of many
Storetests
What do you think?
There was a problem hiding this comment.
i think we could raise a ticket for that cleanup, but probably not worth it now, it'd be a big distraction if its used a bunch in tests...
Using async here though would make sense so we dont add to the problem...
| } | ||
| }) | ||
| .ifExceptionGetsHereRaiseABug(); | ||
| assertThat(isNotBlocked.await(1, TimeUnit.SECONDS)).isFalse(); |
There was a problem hiding this comment.
even something small like 100ms should be plenty here?
… own AsyncRunner when needed
|
@rolfyone finished, ready for review
|
|
Disabled windows test for a while, will check on my local windows |
|
Fixed flakiness with 50a3be1 |
PR Description
I've noticed that blocks are not actually locked if we go in block provider, after inMemory lookup in
retrieveSignedBlock(it could actually changed between) and when directly usingblockProvider, so tried to fix it.Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog