-
-
Notifications
You must be signed in to change notification settings - Fork 261
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
Piping task timeout #243
base: master
Are you sure you want to change the base?
Piping task timeout #243
Conversation
Hmm what if we make it possible to configure the command to not throw on cancellation and instead use |
When executing a command, it's impossible to know how long it will take to complete. If a cancellation token is set and fired while the command is still running, it will force pipingTask an immediate finish, potentially resulting in lost output logs. It's important to note that the cancellation token will only work if it's fired manually by the client, such as in reaction to some expected output on process finish. Otherwise, using To avoid this, it's better to allow some time for the |
So is the difference from the current cancellation behavior such that the |
When you use graceful/forceful cancellation tokens, they send interrupt/kill signal to process, then forceful cancellation token is used to finish piping task. So you could use forceful cancellation token to cancel the process and piping, but then
This PR resolves above issues. |
Closes 241
It resolves the issue, by allowing to specify max timeout to wait for piping to be finished. By doing this way, we can customize how long to wait after the process is finished. The code throws exception, when waiting for piping exceeded timeout, which can be cached by client and it can be properly handled.
Timeout also gives flexibility for the client to give enough time to handle the output, so that it's not lost for the cases when child process is started, but it's important to read all output of the main process.
It's also a lot easier to create nice unit test for the behavior. Unfortunately, it .NET Framework behaves differently in the same case, so I had to exclude it in unit test. In .NET Framework, when main process is exited but child process is still running, the
Exited
event is not fired for some reason.Example from unit test:
Without
.WithPipingTimeout(TimeSpan.FromSeconds(1))
this test gets timeout after 10 seconds.