Lazy initialize process redirection channels - #576
Open
Guest0x0 wants to merge 3 commits into
Open
Conversation
Coverage Report for CI Build 1098Coverage decreased (-0.3%) to 77.967%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
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
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.
Previously, redirection pipes created by
@process.read_from_process,@process.redirect_to_fileetc. are initialized immediately on creation time. As a result, if the program fail before passing the pipes to a child process, the pipes will leak. So users often need to manually inserterrdefer pipe.close()to avoid leak.This PR modifies the semantic by lazy-initialize the pipes inside
@process.run/@process.spawn. This way, if the program fail or jump away before using the pipe, the pipe is not created yet and hence will not leak. Under this new semantic, the child end of aread_from_process(shared=false)/write_to_processpipe never need manual closing.@process.redirect_to_file(shared=false)and@process.redirect_from_filework similarly.@process.pipe()is way more trickier, though. There are two children process involved in@process.pipe(). So the full sequence is:In this PR, pipe initialization is delayed to
spawn child #1, so failure inother setup #1will not result in resource leak. However, ifother setup #2failed, there is no way to close the pipe automatically. So the user need to either avoidother setup #2by reordering code, or handle failure inother setup #2manually. Failure inspawn child #1also has special handling for@pipe.pipe(). Ifspawn child #1failed, both ends of the pipe will be closed.Under the new semantic, code in
@shellcan be simplified, eliminating unnecessary cleanup and book-keeping.I am not sure if this PR should be merged, though. The lazy initialization logic is quite complex, especially in the sharing case. For example, users may read from/write to/close the pipe before it is initialized, and the redirection logic need to handle various kinds of race condition properly. I think this reveals a real weakness in current
@processAPI design. When we mergemoonbitlang/asyncintomoonbitlang/corein the future, perhaps we can re-design the process API to avoid these lifetime problems. No concrete plan yet, though.