Remove unnecessary busy threads reading from processes stdout/stderr #1672
Conversation
This file contains hidden or 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
PProfizi
reviewed
Aug 1, 2024
|
|
||
| # Detect when server is started | ||
| if stdout_line != None and "server started" in stdout_line: | ||
| started = True |
There was a problem hiding this comment.
@ansys-akarcher shouldn't we also break?
There was a problem hiding this comment.
Yeah I was wondering about that also
I don't know if it would be possible to have "server started" and some stderr output
in this case the behavior would be different (breaking would wrongly set started as True, skipping any errors)
|
Hi @ansys-akarcher is this still ongoing work? |
|
@PProfizi the changes were insufficient, as there was also the docker context to take into account. But yeah I'd like to complete these changes at some point |
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.
this PR does not fix anything in particular.
From looking at the code it seems like, when a gRPC server is created, two threads will be started until the server dies, especially if the server startup is successful.
I think the behavior can be kept without the existence of these two threads, as the general approach is: block until the server startup is successful, or an error is detected, or server has not started and time has ran out.
Current modification do not work with server within docker context.