Add option to exit on stdin EOF, standardize wait-for-interrupt, fix runwda exit #159
Conversation
…runwda exit This commit adds a --eof/-e flag to tell go-ios to read stdin and listen for EOF. When run with this flag, EOF (^D) is treated identically to SIGINT (^C). The primary purpose of this modification is to have go-ios exit automatically, with high reliability and in an OS-agnostic way, when a calling program is terminated non-gracefully. When a linux or MacOS process is terminated with SIGKILL, there is no opportunity to kill child processes, which more-than-likely end up as orphans with parent ID 1 (init). This can easily orphan tunnels, wda-instances, etc. However, if go-ios reads from a stdin provided by the parent process, when the parent is killed, the OS is guaranteed to deliver EOF. This is added as a flag (--eof) rather than a default, because it is the default golang cmd.Exec behavior to *immediately* close STDIN (by piping /dev/null) when the user does not explicitly set up an input pipe. Thus, changing the default behviour would likley breaking existing scripts. (In golang, you can force stdin by calling cmd.StdinPipe()). As part of this commit, it was observed that various command-line functions were inconsistent on which signals checked for termination (syscall.SIGINT, syscall.SIGTERM, os.Interrupt). A new common function consolates these calls. This also yields a consistent log statement. Commit 94f24f8 by wangchaoHZ and colerwang, which added a context.Context() option to the "runwda" function also inadvertantly broke clean shutdown on ^C from runwda. The issue was in passing a context.Background() object for the new context parameter, when 'nil' should have been passed instead. Given how this function is implemented, I have attempted to include the appropriate fix, which passes in a new cancellable context that is sensitive to both signals and EOF, and deleted some now-extraneous code. In this instance, it would have been equally correct to simply replace context.Background() with 'nil', but the new approach is more general (since it doesn't require a dedicated cancel function like testmanagerd.CloseXCUITestRunner), and may server as a useful example if contexts are used this other way for functions in the future.
|
Here's a reference for my inspiration for this change request: There are a lot of kludgy OS-specific ways to attempt to make this work (on linux, for example, instead of shelling out to go-ios, we could shell-out to a wrapper shell which reads from stdin, and essentially have the wrapper monitor for failures, terminating go-ios if necessary). But, since go-ios is multiplatform and so are we, I figured it was worth asking for this little addition. It's certainly a non-standard approach, but given how go-ios is used in the wild, I figure this could save headaches for more people than just us. Thanks for your consideration. |
|
Note: tested on MacOS an Linux. Can reproducibly orphan go-ios tunnels and wda instances without this fix by "kill -9" on a parent script. With this mod, I can reliably eliminate orphans by simply subprogging with stdin open to go-ios (even though it is never written to). |
This commit adds a --eof/-e flag to tell go-ios to read stdin and listen for EOF.
When run with this flag, EOF (^D) is treated identically to SIGINT (^C).
The primary purpose of this modification is to have go-ios exit automatically, with high reliability and in an OS-agnostic way, when a calling program is terminated non-gracefully. When a linux or MacOS process is terminated with SIGKILL, there is no opportunity to kill child processes, which more-than-likely end up as orphans with parent ID 1 (init). This can easily orphan tunnels, wda-instances, etc. However, if go-ios reads from a stdin provided by the parent process, when the parent is killed, the OS is guaranteed to deliver EOF.
This is added as a flag (--eof) rather than a default, because it is the default golang cmd.Exec behavior to immediately close STDIN (by piping /dev/null) when the user does not explicitly set up an input pipe. Thus, changing the default behavior would likely breaking existing scripts. (In golang, you can force stdin by calling cmd.StdinPipe()).
As part of this commit, it was observed that various command-line functions were inconsistent on which signals checked for termination (syscall.SIGINT, syscall.SIGTERM, os.Interrupt). A new common function consolidates these calls. This also yields a consistent log statement.
Commit 94f24f8 by wangchaoHZ and colerwang, which added a context.Context() option to the "runwda" function also inadvertently broke clean shutdown on ^C from runwda. The issue was in passing a context.Background() object for the new context parameter, when 'nil' should have been passed instead. Given how this function is implemented, I have attempted to include the appropriate fix, which passes in a new cancellable context that is sensitive to both signals and EOF, and deleted some now-extraneous code. In this instance, it would have been equally correct to simply replace context.Background() with 'nil', but the new approach is more general (since it doesn't require a dedicated cancel function like testmanagerd.CloseXCUITestRunner), and may server as a useful example if contexts are used this other way for functions in the future.