Extending threadIsMain() assert
#4613
Assignees
Labels
Comments
|
I need this for some of the background transaction queue work, so I'm implementing it as part of that work |
bboston7
added a commit
to bboston7/stellar-core
that referenced
this issue
Feb 11, 2025
Closes stellar#4613 This change adds a function `threadIsType` function to `Application` that allows a thread to check its type. This is intended to support more detailed assertions than the usual `releaseAssert(threadIsMain())` assertions we're currently using everywhere. The implementation differs a bit from the proposed solution in stellar#4613 as it uses a mapping in `ApplicationImpl` to track thread types, rather than using static variables. I chose this approach because as far as I can tell, it's not possible to assign a thread an id. Given that, `ApplicationImpl` would need to set these variables in its constructor, and the variables would hold meaningless values prior to that. I figure it's safer to ensure that thread types can only be reasoned about after the creation of the threads themselves in `ApplicationImpl`'s constructor. However, if it's important that thread types be reasoned about without an `Application` or `AppConnector`, then I'm open to changing the design.
6 tasks
bboston7
added a commit
to bboston7/stellar-core
that referenced
this issue
Feb 11, 2025
Closes stellar#4613 This change adds a function `threadIsType` function to `Application` that allows a thread to check its type. This is intended to support more detailed assertions than the usual `releaseAssert(threadIsMain())` assertions we're currently using everywhere. The implementation differs a bit from the proposed solution in stellar#4613 as it uses a mapping in `ApplicationImpl` to track thread types, rather than using static variables. I chose this approach because as far as I can tell, it's not possible to assign a thread an id. Given that, `ApplicationImpl` would need to set these variables in its constructor, and the variables would hold meaningless values prior to that. I figure it's safer to ensure that thread types can only be reasoned about after the creation of the threads themselves in `ApplicationImpl`'s constructor. However, if it's important that thread types be reasoned about without an `Application` or `AppConnector`, then I'm open to changing the design.
github-merge-queue bot
pushed a commit
that referenced
this issue
Feb 12, 2025
Closes #4613 This change adds a function `threadIsType` function to `Application` that allows a thread to check its type. This is intended to support more detailed assertions than the usual `releaseAssert(threadIsMain())` assertions we're currently using everywhere. The implementation differs a bit from the proposed solution in #4613 as it uses a mapping in `ApplicationImpl` to track thread types, rather than using static variables. I chose this approach because as far as I can tell, it's not possible to assign a thread an id. Given that, `ApplicationImpl` would need to set these variables in its constructor, and the variables would hold meaningless values prior to that. I figure it's safer to ensure that thread types can only be reasoned about after the creation of the threads themselves in `ApplicationImpl`'s constructor. However, if it's important that thread types be reasoned about without an `Application` or `AppConnector`, then I'm open to changing the design. # Checklist - [x] Reviewed the [contributing](https://github.com/stellar/stellar-core/blob/master/CONTRIBUTING.md#submitting-changes) document - [x] Rebased on top of master (no merge commits) - [x] Ran `clang-format` v8.0.0 (via `make format` or the Visual Studio extension) - [x] Compiles - [x] Ran all tests - [ ] If change impacts performance, include supporting evidence per the [performance document](https://github.com/stellar/stellar-core/blob/master/performance-eval/performance-eval.md)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Given the amount of parallelism being added to core with #4543, we now have many different types of threads responsible for different tasks. Binary asserts of
!threadIsMain()andthreadIsMain()no longer hold as much meaning, and we've had to loosen our thread asserts in a lot of places. We should extend thethreadIsMainassert to all different "thread types," such as overlay thread, apply thread, etc. This shouldn't be too difficult. For each thread type we cal declare a 0 initializedthread::idlike we currently do formainThread, then we can initialize the value when spinning up the thread. It may also be useful to rename the main thread to something like "consensus thread" or "scheduling thread" so it carries a little more meaning.The text was updated successfully, but these errors were encountered: