Add api to signal whether Stream owns or borrows an FD#25
Merged
Conversation
dcbaker
changed the title
The Stream class has always assumed that it has ownership of the file-like object it is writing into. This isn't (and can't be) true in some cases, like writing to stdout. This commit adds a new argument to the Stream class, close_fd, which signals to the Stream class whether it owns the fd or not, and thus whether it should close it. Fixes: #23
This would have happened in fd.close(), and if the fd happens to be in in use multiple places then you can get mixed output. While it probably makes sense if you control the fd to create a locking wrapper, this is difficult for things like `sys.stdout`, which would require monkey patching to guard with locks if using any external modules (including the standard library). Fixes: #24
dcbaker
force-pushed
the
submit/dont-close-an-fd-we-didnt-open
branch
from
c839b20 to
b657cb0
Compare
Owner
Author
|
@JonoYang, Does this solve your issues? |
Contributor
Owner
Author
|
I'm not sure when the next release will be. I'm still thinking about #20. but probably soonish. |
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
2 participants
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.
The Stream class assumes that it owns the fd that it's writing into, which is only assured to be true when it opens the fd itself. If it didn't open the fd, it may not be appropriate for it to close it. This is not the ideal solution, the ideal would be that if you pass an fd you have to tell us Stream if it owns the fd or not. But that would be a backwards incompatible change.
The solution this uses then is to create an api with a deprecation warning. If you don't tell Stream we assume we own it, but in 1.0 that will switch and we'll assume the caller owns it, and we're just borrowing it.
Fixes: #23
Fixes: #24