Skip to content
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

let forkChoice onTick fail #8096

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Mar 18, 2024

*** NEEDS TO BE CAREFULLY ANALYZED ***

If there are possible deadlock conditions, we should avoid "joining" forever and let timeout so we can allow deadlock to resolve. We should double-check and, if we fail, we remain in a recoverable state, not in a broken-corrupted state.

tickProcessor.onTick(currentTimeMillis).get(30, TimeUnit.SECONDS);
} catch (final InterruptedException | TimeoutException | ExecutionException e) {
throw new RuntimeException(e);
}
performanceRecord.ifPresent(TickProcessingPerformance::tickProcessorComplete);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's worth doing this finish in an 'always' block

also, we should probably put in a counter for failed ticks...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, right

@rolfyone
Copy link
Contributor

potentially given the stacktrace we looked at, raised #8097 as an alternative

@rolfyone
Copy link
Contributor

i think if we do this, we'd plumb it in so we can tweak the value, and we'd start it high. I like the idea that timedTick can timeout, but being able to alter that value if its too low sometimes would be much easier if we could do it with cli arg...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants