[core] checking the status of thread creation #36510
Open
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
It is possible for allocation of a system resource to fail. While the Thread abstraction in the core allows for a 'success' parameter, it is optional and not generally checked where used. This can lead to degenerate behavior which is hard to diagnose. In particular, the Executors assign Closures to their queues regardless of whether the thread handling to queue is actually running, leading those async requests to hang. This change makes the 'success' parameter required and tries to make the correct decision on how to proceed in the code where those Threads are started. For the Executor, we will attempt to carry on with the number of threads we were able to create. In other areas, we assert, assuming that whatever function is being performed by the allocated thread is critical to the overall function of the library. For the tests, we ignore the value of 'success' as we assume the tests will fail if thread creation failed. This is no different than the existing behavior.
|
Resubmitting original PR: #33286 |
veblush
approved these changes
May 7, 2024
veblush
added
the
release notes: no
grpc-checks
bot
added
bloat/none
per-call-memory/decrease
per-channel-memory/neutral
labels
veblush
reviewed
May 14, 2024
| /// is successfully created. | ||
| /// The optional \a options can be used to set the thread detachable. | ||
| Thread(const char* thd_name, void (*thd_body)(void* arg), void* arg, | ||
| bool* success = nullptr, const Options& options = Options()); | ||
| bool* success, const Options& options = Options()); |
There was a problem hiding this comment.
We still need old way of creating Thread because internal code (that you cannot access) still need this.
There was a problem hiding this comment.
Adding a new constructor that does not require a success argument would undermine the point of this change. It would allow future code to use this constructor and thereby re-introduce the problem we have that pthread_create might fail and the result is a silent, slow degradation of functionality which is very difficult to diagnose.
There was a problem hiding this comment.
It'd be true but we cannot accept it without having a way to not break internal codes.
veblush
reviewed
May 14, 2024
|
|
||
| Thread(const char* thd_name, absl::AnyInvocable<void()> fn, | ||
| bool* success = nullptr, const Options& options = Options()) | ||
| Thread(const char* thd_name, absl::AnyInvocable<void()> fn, bool* success, |
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.
It is possible for allocation of a system resource to fail. While the Thread abstraction in the core allows for a 'success' parameter, it is optional and not generally checked where used. This can lead to degenerate behavior which is hard to diagnose. In particular, the Executors assign Closures to their queues regardless of whether the thread handling to queue is actually running, leading those async requests to hang.
This change makes the 'success' parameter required and tries to make the correct decision on how to proceed in the code where those Threads are started. For the Executor, we will attempt to carry on with the number of threads we were able to create. In other areas, we assert, assuming that whatever function is being performed by the allocated thread is critical to the overall function of the library.
For the tests, we ignore the value of 'success' as we assume the tests will fail if thread creation failed. This is no different than the existing behavior.