Add function to allow checking thread type #4645
Merged
+44
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
graydon
approved these changes
Feb 11, 2025
SirTyson
requested changes
Feb 11, 2025
graydon
requested changes
Feb 11, 2025
auto-merge was automatically disabled
February 11, 2025 23:04
Head branch was pushed to by a user without write access
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.
bboston7
force-pushed
the
thread-asserts
branch
from
894c5fd to
d1ae2d7
Compare
Done |
graydon
approved these changes
Feb 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4613
This change adds a function
threadIsTypefunction toApplicationthat allows a thread to check its type. This is intended to support more detailed assertions than the usualreleaseAssert(threadIsMain())assertions we're currently using everywhere.The implementation differs a bit from the proposed solution in #4613 as it uses a mapping in
ApplicationImplto 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,ApplicationImplwould 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 inApplicationImpl's constructor. However, if it's important that thread types be reasoned about without anApplicationorAppConnector, then I'm open to changing the design.Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)