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

Ensure that switchover vars are initialized before use: #4527

Merged
merged 1 commit into from
May 23, 2023

Conversation

seelabs
Copy link
Collaborator

@seelabs seelabs commented May 18, 2023

Global variables in different TUs are initialized in an undefined order. At least one global variable was accessing a global switchover variable. This caused the switchover variable to be initialized in an incorrect state.

Since the switchover is always explicitly set before transaction processing, this bug can not effect transaction processing, but could effect unit tests (and potentially the value of some global variables). Note: at the time of this patch the offending bug is not yet in production.

Type of Change

  • [x ] Bug fix (non-breaking change which fixes an issue)

Global variables in different TUs are initialized in an undefined order.
At least one global variable was accessing a global switchover variable.
This caused the switchover variable to be initialized in an incorrect
state.

Since the switchover is always explicitly set before transaction
processing, this bug can not effect transaction processing, but could
effect unit tests (and potentially the value of some global variables).
Note: at the time of this patch the offending bug is not yet in production.
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

Nice job!

Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@seelabs seelabs added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 21, 2023
@intelliot intelliot added Bug and removed Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. labels May 22, 2023
@a-noni-mousse
Copy link
Contributor

Why not use the thread_local compiler support for the code?

@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 22, 2023
@intelliot intelliot added this to the 1.11.0 milestone May 22, 2023
@HowardHinnant
Copy link
Contributor

I think using thread_local is something we should strongly consider, but definitely not in this PR.

We use LocalValue in a few places and we should consider a PR which moves all of them to thread_local (and does nothing else). This would be a good move to reduce our technical debt.

But #4527 should remain just about fixing the bug without introducing further complications which would risk introducing a bug.

@seelabs
Copy link
Collaborator Author

seelabs commented May 23, 2023

@a-noni-mousse @HowardHinnant LocalValue is meant to work with coroutines and thread_local isn't going to play well with co-routines. Having said that, I'm not sure any co-routines are actually using LocalValue. If we audit the code and it's not used we may be able to replace it, but even then we'd have to make sure it was never mistakenly used with a co-routine.

@intelliot intelliot merged commit ce997a6 into XRPLF:develop May 23, 2023
@intelliot intelliot mentioned this pull request May 23, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants