Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork937
Description
When callingGit.execute, including indirectly through the dynamic methods of aGit object, passing a non-None value ofoutput_stream suppresses the effect ofkill_after_timeout.Thethreading.Timer object is created with the localkill_process function as its callback, butstart is never called on it.
Lines 1050 to 1079 ina58a6be
| ifoutput_streamisNone: | |
| ifkill_after_timeoutisnotNone: | |
| watchdog.start() | |
| stdout_value,stderr_value=proc.communicate() | |
| ifkill_after_timeoutisnotNone: | |
| watchdog.cancel() | |
| ifkill_check.is_set(): | |
| stderr_value='Timeout: the command "%s" did not complete in %d '"secs."% ( | |
| " ".join(redacted_command), | |
| kill_after_timeout, | |
| ) | |
| ifnotuniversal_newlines: | |
| stderr_value=stderr_value.encode(defenc) | |
| # Strip trailing "\n". | |
| ifstdout_value.endswith(newline)andstrip_newline_in_stdout:# type: ignore | |
| stdout_value=stdout_value[:-1] | |
| ifstderr_value.endswith(newline):# type: ignore | |
| stderr_value=stderr_value[:-1] | |
| status=proc.returncode | |
| else: | |
| max_chunk_size=max_chunk_sizeifmax_chunk_sizeandmax_chunk_size>0elseio.DEFAULT_BUFFER_SIZE | |
| stream_copy(proc.stdout,output_stream,max_chunk_size) | |
| stdout_value=proc.stdout.read() | |
| stderr_value=proc.stderr.read() | |
| # Strip trailing "\n". | |
| ifstderr_value.endswith(newline):# type: ignore | |
| stderr_value=stderr_value[:-1] | |
| status=proc.wait() | |
| # END stdout handling |
This situation appears not entirely intentional, because:
- The "watchdog"
Timeris created, even though it is never used. - The docstring, even as it documentsthat
as_processsuppresseskill_after_timeout(and thatas_processsuppressesoutput_stream), does not mention any effect ofoutput_streamonkill_after_timeout.
So eitherkill_after_timeout should be honored even whenoutput_stream is used,or the docstring should be updated to mention that it does not (and the code possibly modified to not create theTimer object in the case that it is unused).
I am not sure which change should be made, so I'm opening this rather than a PR to propose one of them.
This is effectively separate from#1756. (One quality this shares with#1756 is that it is relevant only to the behavior ofGit.execute and functions that use it. Functions that usehandle_process_output are unaffected; itskill_after_timeout is implemented differently with nonequivalent behavior and separate code.handle_process_output also does not take anoutput_stream argument.)