-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
libnative: Convert non-blocking fds to blocking when necessary #13355
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
-include ../tools.mk | ||
|
||
all: | ||
$(RUSTC) write.rs | ||
$(CC) -o $(TMPDIR)/mknblock mknblock.c | ||
mkfifo $(TMPDIR)/fifo | ||
/bin/sh -c "(sleep 1; cat) < $(TMPDIR)/fifo > /dev/null" & | ||
/bin/sh -c "($(TMPDIR)/mknblock; $(TMPDIR)/write) > $(TMPDIR)/fifo" |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
#include <fcntl.h> | ||
#include <stdio.h> | ||
|
||
int main() { | ||
if (fcntl(1, F_SETFL, O_NONBLOCK) == -1) { | ||
perror("fcntl"); | ||
return 1; | ||
} | ||
return 0; | ||
} |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
use std::io; | ||
use std::io::IoResult; | ||
use std::os; | ||
|
||
fn run() -> IoResult<()> { | ||
let mut out = io::stdio::stdout_raw(); | ||
for _ in range(0u, 1024) { | ||
let mut buf = ['x' as u8, ..1024]; | ||
buf[1023] = '\n' as u8; | ||
try!(out.write(buf)); | ||
} | ||
Ok(()) | ||
} | ||
|
||
fn main() { | ||
match run() { | ||
Err(e) => { | ||
(writeln!(&mut io::stderr(), "Error: {}", e)).unwrap(); | ||
os::set_exit_status(1); | ||
} | ||
Ok(()) => () | ||
} | ||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said earlier, I'm a little uncomfortable blanket applying this to all file descriptors. We are only aware of this problem in connection with the stdout file descriptors.
Additionally, I think the interface may need to be tweaked slightly. The errno of the read/write syscall is currently overwritten when fcntl fails. I also think that this shouldn't cause an infinite loop, I would expect this to not retry if O_NONBLOCK wasn't set before, and I would only expect it to retry once afterwards. Is there a reason to continually try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stdin and Stderr too definitively have this problem. And I fully expect it's possible to get the same problem with other file descriptors, e.g. by explicitly opening a terminal device. And of course you can
dup2()
stdout and get an fd that!= 1
and yet I believe it will share theO_NONBLOCK
flag.Also, if we don't have this problem with other fds, then they'll never trigger this code.
The errno of read/write being overwritten is unfortunate. However, according to the man pages, the only errors that
fcntl()
can return forF_GETFL
andF_SETFL
areEAGAIN
/EWOULDBLOCK
andEBADF
(a linux manpage indicatesEPERM
is possible, but only when clearingO_APPEND
and we're not doing that). And of courseEBADF
is a legitimate error fromread
/write
as well, so I'm not worried about it.I don't believe this will cause an infinite loop, unless there's some way the system can return
EAGAIN
/EWOULDBLOCK
when theO_NONBLOCK
flag is not set. I checked and the only other way I found for that to happen is withO_NDELAY
, which a) isn't POSIX, and b) seems to be equal toO_NONBLOCK
on everything I looked at except for Linux on sparc, which we don't support. I could certainly add in code to ensure that the flags do haveO_NONBLOCK
set, but I don't expect that code to ever catch anything.Continually retrying is necessary. Fds are a shared resource (certainly among threads, and as this bug indicates, certain file descriptors share their flags among processes). So libnative could remove
O_NONBLOCK
only to have something else immediately set it again before libnative has a chance to re-enterread()
/write()
. Therefore, doing this in an infinite loop is necessary. If it makes you feel any better, it makes progress on every pass through the loop, and it will eventually end (and of course it should only ever need a single retry in pretty much every single case. I will be pretty surprised if anyone actually manages to need multiple passes through the loop).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it some more, bailing if
O_NONBLOCK
isn't set is technically dangerous, because some other process could have unset it before we calledfcntl()
, and then could set it again afterwards. This of course is only a problem if we check for the presence ofO_NONBLOCK
first.O_NONBLOCK sucks. POSIX really should have just declared a
read_nonblock()
andwrite_nonblock()
.Granted, this particular case should never be hit in practice. So I guess I can live with this edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just this morning I was sure that libnative would never see EAGAIN. Just a week ago I was sure that
kill
would fail if a process had died. Please handle errno(), defensive programming I've found is far better than assuming everything will go right.In the spirit of defensive programming, please remove the loop. The code is no less readable, and failing in an infinite loop (albeit rarely) is arguably much worse than returning an error from print. This is not an iron-clad solution against failing prints, this is simply trying to alleviate a bug which has been sporadically seen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcrichton This isn't about assuming it will go right. This is about the documented errors that
fcntl()
can return. According to POSIX the only valid error it can return here matches an error thatread
andwrite
would return (for the same reason). And Linux documents a second error that I don't believe we can ever hit.The problem with trying to handle errno correctly is that we can't simply save errno and restore it to what it was before the fcntl. And trying to change things such that
blocking()
returns theIoResult
directly is awkward. The one thing I could do is re-issue the call toread
/write
, and I'm willing to do that if it makes you happy, but I just don't think it's necessary.No. That's absolutely incorrect. The loop makes progress on every single pass through it (either by succeeding, or by removing the
O_NONBLOCK
flag). Removing the loop makes this code wrong. It in fact exists because of the spirit of defensive programming. It's defending against a rare race condition where some other thread/process just happens to flipO_NONBLOCK
back on at the wrong time.Removing the loop here would be like removing the loop in an atomic.fetch_and_add() implementation because you don't want that to spin forever.