os: Missed checking for EINTR in (*os.File).readdir #40846
Comments
bcmills
added
the
NeedsInvestigation
|
As far as I know neither the Can you tell us more about the exact case where this fails? Can you show us a way to reproduce the problem? Thanks. |
|
@ianlancetaylor thanks for the quick reply!
So the underlying mount is a FUSE filesystem and the it seems like returning There's a lot of other logs around this so I'm under the current assumption that the
Once we can determine what the signal is that caused this, then I can hopefully come up with some sort of simple reproduction. We can just make a simple FUSE program that just sleeps and then trigger the signal and it should send an Interrupt. [1] https://www.kernel.org/doc/Documentation/filesystems/fuse.txt |
|
Thanks for the info. The signal was most likely You should be able to see it by running the program under |
|
@fastest963 what FUSE library are you referring to? How is it configured on your system? That's probably all we need to know to reproduce it. Go 1.14+ generates a lot of interrupts, for goroutine scheduling. Causing an interrupt is easy in a Go app. |
|
@networkimprov It's using https://github.com/bazil/fuse to mount the FUSE mount. Nothing very special on the configuration front other than It should print out:
Here's the relevant strace output: and the associated FUSE logs: Looks like this happened inside of |
|
Thanks. Can you double check that you passed the |
|
@ianlancetaylor I actually used The relevant section is: |
|
cc @tv42 |
|
@fastest963 Thanks. The strace output shows that the I guess I'll just look for syscall functions in the os package and add |
|
Change https://golang.org/cl/249178 mentions this issue: |
|
A non-FUSE alternative for testing would be seccomp or ptrace triggering that EINTR. Or a special slower build where RawSyscall etc check a flag and never even enter kernelspace. Of course both of those need to be somehow targeted to the thing under test, matching by fd or pathname or kernel thread id. And yes once networking etc are added to the mix, just about anything can return EINTR, because they mostly have no other choice -- allowing user to control-C out of slow networking is a nice thing to have, so not bailing out isn't really an option. Only a precious few syscalls have something else meaningful to report, so EINTR it is. If Go's scheduler using SIGURG triggers this logic, that could lead to a nasty scenario where the work is started from scratch on every round, and always interrupted by SIGURG. I sure hope the SIGURG doesn't happen too often. |
|
In that case, maybe loop-on-EINTR logic should be disabled via an os.OpenFile() flag. That would apply to previously added EINTR loops, too. We need loops to keep existing code working with async-preemption, and also need to accommodate network filesystems. |
|
If you need to open a file with the possibility of interruption or cancellation, an explicit |
|
I've added a comment on the FS API thread re network filesystems and EINTR loops: |
|
I don't think there should be a special flag to os.OpenFile; the default and only behavior should be "do the right thing" (whatever that ends up meaning). I posit the two requirements for developer and user comfort here are:
The way to differentiate between cases 1 and 2 is the SIGINT, not the EINTR. |
|
Assuming a reliable method to determine the source signal for every occurrence of EINTR, on every OS that returns it :-) Programs may expect EINTR for more than SIGINT, so you'd retry on EINTR when the cause was SIGURG. |
|
We don't need to know what signal caused the We always send |
|
I'm not sure you can just retry on all EINTR and call it a day. That means one can't write a SIGINT handler that does an orderly shutdown, because it can't claw back control of those threads that are in syscalls. If you leave it at this, all such shutdown is done by essentially abandoning the goroutines that are hanging in syscalls. I don't think this is a good idea, and I don't think it's always doable safely! The hang might have been e.g. I believe that programs should be able to receive SIGINT, un-hang those syscalls that can be un-hung, and do an orderly shutdown. There are plenty of examples of this in the C world. The typical convention is that a second SIGINT while trying to do an orderly shutdown makes the program crash harder. I posit that the only way to achieve "orderly shutdown" is by letting the syscalls complete (hopefully with EINTR asap; one may need to nudge them that way!), and then let the calling code deal with the consequences, with the top level waiting on something like errgroup until everything is shut down. A single-threaded C program would set a global flag ("shutting down") in a signal handler, and then the EINTR loop would actually be A Go program doesn't even get to have signal handlers that are guaranteed to run before the EINTR loop goes back to making the syscall hang. And that's before we even consider multithreading, where the hanging syscall(s) are in separate threads from where the signal is handled! A multithreaded program will likely end up getting the SIGINT on thread different from the thread that's hanging on a syscall. Hopefully the scheduler will keep something available to run a goroutine to actually handle the signal, these are not considered "blocking syscalls" afaik! This multi-threaded aspect is making me think there's basically only one path forward to clean shutdowns: Some sort of universal support for To make this happen, the hanging syscalls can be woken up with a signal (assuming kernel support again), and EINTR is a perfectly fine error in such a scenario. A multi-threaded program would have to "broadcast" the incoming signal (handled by just one thread) to all of its threads, to wake up all stuck syscalls. Here's a strawman proposal: Add function to make all
(It may also send signals to more threads than that, just to make implementation simpler. Goroutines not in syscalls would not observe anything special happening; pretty much exactly like the scheduler SIGURG code path.) These signals will not be seen specifically by Construct a mechanism for InterruptAll to disable EINTR retry loops exactly once, so we don't wreck the continuing existence of the program with spurious EINTRs. Something like: Have a per-goroutine |
|
Thanks for the comment. However, I don't think what you are suggesting is a good fit for the Go world. Go programs have a managed runtime. One consequence is that they can in effect crash at any time, for example if the runtime is unable to allocate memory or unable to start a new thread. There is no reliable way to enforce the orderly shutdown of a Go program. This is an intentional choice made by the language and runtime. For cases where an orderly shutdown is essential, Go requires separating the program into separate processes. For example, the parent process can run the real program in a child process and then, if the child process exits in some unpredictable way, restore the state to some acceptable status. |
|
I think crash safety and orderly shutdown are two different things. I'm a huge proponent of even crash-only software overall, but there are cases where an orderly shutdown just makes sense. I think we can keep "user asks to end program" quite separate from "out of memory". There's no way to enforce the orderly shutdown of anything computerized, but that doesn't mean all hope is lost for the common case. Here's a recent example of graceful shutdown functionality added to stdlib: https://golang.org/pkg/net/http/#Server.Shutdown |
|
@tv42 maybe you could paste your previous comments into a new proposal issue? We'd get more eyes on it that way. |
|
@tv42, we have an existing mechanism for tearing down interrupted calls. That mechanism is If we had a way to plumb the |
|
Yes, the advantage of using |
|
@bcmills That would require plumbing Context to an |
|
@tv42 I don't see a reason to worry about whether the actual system call returns or not. That doesn't matter with regard to an orderly shutdown. It's fine for the program to exit with a bunch of threads stuck in system calls. The kernel will clean them up. What matters for an orderly shutdown is whether the goroutines complete their logical shutdown. And that can be done, in principle, with a |
|
Sorry, I must have missed something... How does a |
|
@tv42 Fair point, please read all discussions about deadlines above as using contexts (https://golang.org/pkg/context). The issues seem the same. |
|
Ian, another way to look at the problem is that EINTR is part of the API for CIFS & FUSE, because they're designed to be user-interruptible. This is not compatible with Go, so we'd like to fix that. Syscall interruption by signalling a thread requires runtime support, so wouldn't the API reside in stdlib? See my suggested mechanism in #40846 (comment). There's no need to replicate the file API with a Context or deadline argument. This is sufficient: This is an Shall I post a proposal for this? |
That is entirely reasonable for C, but it does not make sense for Go, because Go is always multi-threaded. When you send a signal to a Go program, it is unpredictable whether it will interrupt a system call or not, because it is unpredictable which of the various threads will receive the signal. The only way to make that predictable is to direct the signal to a specific thread. But the only code that can do that reliably is code within the program itself. Perhaps there are cases where a Go program should interrupt a specific thread. But even if we believe that, then the error returned to the user's program for that thread should not be |
I suspect that we could do everything via the golang.org/x/sys/unix package, but I could be missing something.
That is not going to be a good API, because it will require every single file operation to register itself somewhere so that it can be interrupted, and then deregister itself when complete. That is too high a price to pay for a feature that extremely few programs will want to use. |
|
Strawman API proposal: (Or, alternatively, This could be done by piggybacking on (This is starting to smell a lot like implicit context passing, #28342 and probably others -- but it would avoid duplicating every API just to add ctx.) |
|
Frankly I wouldn't hesitate to duplicate APIs to add a context. Explicit is better than implicit. I'm not inherently opposed to using a signal to interrupt the thread but it's not like we can just send a signal and carry on. What if the thread isn't in a system call? What if the thread is preempted by the kernel but just about to enter a system call when it resumes? Maybe a signal can be part of the code but it needs considerable machinery around it. |
When a network filesystem becomes unavailable, you interrupt all syscalls trying paths within a file tree whose root is on the network. Marking as interruptible all file ops in the current function (and implicitly, those in functions it calls, etc) isn't helpful, as you need to interrupt some ops and not others, depending on their location. Interrupt errors would often trigger context cancellation. A file tree is typically used by many tasks; it's not a task unto itself.
Thankfully, no. The
@aclements is probably better able to address these Qs. (Austin, the current subthread starts at #40846 (comment).)
We also need this on Windows for CIFS. The x/ tree is fine, but not x/sys.
Any program that relies on the filesystem and is deployed into environments that its developers don't control needs to allow for network filesystems. They're still heavily used in office LANs. |
Since the scheduler sends SIGURGs anyway, SIGURG should be harmless. Either the scheduler knows that it didn't mean to preempt that goroutine, and the signal handler does nothing, or the goroutine goes to sleep for a while. Neither should be visible to the program, and these false positive SIGURGs shouldn't happen often enough to cause much trouble.
Good question. This is sort of comparable to the race inherent in Basically what is being asked here is to reconcile the desire ("cancel the slow operation in this goroutine") with the current state ("this goroutine is not / is inside a syscall"), and sometimes that sort of a reconciliation needs to be repeated until it reaches its desired effect. One thing that needs careful thought is aiming the cancellation at the correct syscall. Consider Strawman implementation idea 1: Waste a goroutine per cancelable syscallStrawman implementation idea 2: Add a context field and a periodic jobScheduler "G" goroutine state (type New function
Have a periodic job that scans for G's in state (Alternatively, instead of (Might also be able to relocate |
Sorry, I didn't mean that user code would have to register. I meant that the each file operation in the standard library would have to register itself. As I said earlier, that is too high a price to pay for a feature that extremely few programs will want to use.
That is easy to say, but how many actual programs actually do that, in any language? |
|
@tv42 For read-only operations I would vote for what I suggested above in #40846 (comment), but add some mechanism to clean up the system call in the background. I don't see an obvious reason to force the real work of the program to wait until the system call is interrupted. For write operations I agree that we would need to somehow know whether the system call completes or is interrupted. |
|
Leaving the syscall running would leave the buffer passed to Read etc poisoned, so that would only apply to things that allocate internally, and have no out-parameters. I'm also uncomfortable with leaking such threads-in-syscalls. There's no guarantee when or even if they will return. It just seems like a bad design to me. I'd much rather have the user program in charge of them, to have that visibility. |
|
Either way, it seems clear to me that this use-case needs a proposal with more depth to it than just “don't check for (But again, I wouldn't expect to make progress on any API changes until we have a clearer picture of where #5636 is going.) |
I understood what you meant. It's addressed by either of the approaches I gave (quoted above). All: a core issue is what the runtime must do to let a blocked syscall thread be signaled on demand. We should find a general-purpose scheme. I see two options... a) keep a table of syscall thread IDs, adding to it when requested; provide a reference or copy of it when requested PS: discussion of #5636 has moved to https://www.reddit.com/r/golang/comments/hv976o/qa_iofs_draft_design/?sort=new |
|
I filed #41054 after digesting the above discussion. It should support both use cases described above: a) APIs that can be cancelled via a |
I'm sorry for a late post here, but I do have an example of this kind of a workload. I'm using NFS-based disk as a write-through cache for large objects in front of DynamoDB. My code first tries to check the contents on the disk with a very tight deadline (2ms) and if the deadline expires it falls back on an expensive call into DynamoDB. As such, it needs timeouts for all calls used and stat is among them (to check for object's presence). |
|
Alex, curious to hear what OS is running the NFS client, and which client if not the default? Last I checked Linux doesn't interrupt syscalls to NFS on signal. |
Do you mean: you do a We already support setting a deadline on a |
|
Correct, we do all operations (including read() and write()) under tight deadlines and if these deadlines are violated we fall back to the direct database requests. These requests are expensive in monetary terms, as DynamoDB essentially bills per request. In case of read() and write() it's a bit more involved, we "recharge" the deadline upon receiving each 512Kb of data. The stat() calls are used to get the object size and the creation time, we don't use readdir() right now. We use regular IO in a child goroutine and simply abandon it in case the deadline expires. We also added a semaphore to limit the concurrency in case of NFS server hiccups. It all works acceptably well, but it would be nice to actually be able to just be able to use the regular context-based cancellation. |
Linux and NFSv4.
Linux supports interrupting NFS calls: https://elixir.bootlin.com/linux/latest/source/net/sunrpc/sched.c#L924 and https://elixir.bootlin.com/linux/latest/source/net/sunrpc/sched.c#L954 - but so far it's used only during unmounting. I'm planning to fix that in the upstream Linux. |
|
@Cyberax I'm curious as to why NFS vs HTTP or somesuch? |
|
@bcmills @ianlancetaylor Is the fix for this https://go-review.googlesource.com/c/go/+/249178/ going to get a backport into go1.15.x ? I couldn't see it on the milestones. |
|
@ncw The problem is not new in 1.15, so this does not qualify for a backport by our usual rules. |
|
To clarify, for the great majority of Go sites, the problem is new in 1.14. It can in theory occur before that, but was never reported. |
Ah, that's a shame. Rclone users seem to come across this quite regularly. |
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
See: https://go-review.googlesource.com/c/go/+/232862/
If you call
ioutil.ReadDirit ends up calling(*os.File).readdirwhich callslstatand does not retry on an EINTR error. Other spots were handled in https://go-review.googlesource.com/c/go/+/232862/ and I think this spot was just missed.I wasn't sure of the best way since
lstatis automatically generated. Should this just be handled inreaddir?What did you expect to see?
I expected
ioutil.ReadDirnot to error.What did you see instead?
Instead
ioutil.ReadDirerrored withlstat /opt/admiral-dev/hazadblock/dist/static/lib/ergoHeadOverwrite.dev.min.js.map: interrupted system call.The text was updated successfully, but these errors were encountered: