| Message ID | 1475843186-3429-1-git-send-email-blomqvist.janne@gmail.com |
|---|---|
| State | New |
| Headers | show |
> Many POSIX systems have the bad habit of not restarting interrupted > syscalls. On these systems it's up to the user to check for an error > with errno == EINTR and restart manually. This patch does this for > libgfortran, so that GFortran users don't have to do it. I have not much experience with EINTR, but is it garanteed that those EINTR loops will never cycle forever? Apart from that, OK to commit. FX
On Fri, Oct 7, 2016 at 2:41 PM, FX <fxcoudert@gmail.com> wrote: >> Many POSIX systems have the bad habit of not restarting interrupted >> syscalls. On these systems it's up to the user to check for an error >> with errno == EINTR and restart manually. This patch does this for >> libgfortran, so that GFortran users don't have to do it. > > I have not much experience with EINTR, but is it garanteed that those EINTR loops will never cycle forever? Hmm, no I don't think so, but I don't think it'll be a problem. So on systems where syscalls are not restarted automatically, EINTR happens when the process receives a signal while blocked in a system call [1]. So I suppose in theory you could have a situation where something continuously fires signals at the process, and the result is some kind of race between the process restarting the syscall which then never manages to complete before being interrupted again. But I think this goes into the "Doctor, this hurts! Then don't do that" territory. There's some more info in https://www.python.org/dev/peps/pep-0475/ (Python nowadays does the same as this patch). > > Apart from that, OK to commit. > > FX
On Fri, Oct 7, 2016 at 8:59 AM, Janne Blomqvist <blomqvist.janne@gmail.com> wrote: > On Fri, Oct 7, 2016 at 2:41 PM, FX <fxcoudert@gmail.com> wrote: >>> Many POSIX systems have the bad habit of not restarting interrupted >>> syscalls. On these systems it's up to the user to check for an error >>> with errno == EINTR and restart manually. This patch does this for >>> libgfortran, so that GFortran users don't have to do it. >> >> I have not much experience with EINTR, but is it garanteed that those EINTR loops will never cycle forever? > > Hmm, no I don't think so, but I don't think it'll be a problem. So on > systems where syscalls are not restarted automatically, EINTR happens > when the process receives a signal while blocked in a system call [1]. > So I suppose in theory you could have a situation where something > continuously fires signals at the process, and the result is some kind > of race between the process restarting the syscall which then never > manages to complete before being interrupted again. But I think this > goes into the "Doctor, this hurts! Then don't do that" territory. > > There's some more info in https://www.python.org/dev/peps/pep-0475/ > (Python nowadays does the same as this patch). Just one concern (slightly different from the race you described) - what if a user wants/expects a system call to be interrupted? With the patch we would always restart the system call even if the user was expecting it would be interrupted. For small calls like lseek this may not be a big deal but if read on a pipe/socket/terminal is restarted after being interrupted (e.g. with CTRL-C) we may loop forever even if the user program was written to expect and handle EINTR after the read call, e.g. to terminate nicely with "non async-safe" calls like printf that couldn't be done in the handler. This is discussed as "use case 2" in the PEP you referenced. Python handles this case by explicitly calling user defined signal handlers directly after EINTR and checking the return value from the handler, only trying again if the handler reports success. Not so simple I think with libgfortran. --- Fritz Reese
On Fri, Oct 7, 2016 at 5:50 PM, Fritz Reese <fritzoreese@gmail.com> wrote: > On Fri, Oct 7, 2016 at 8:59 AM, Janne Blomqvist > <blomqvist.janne@gmail.com> wrote: >> On Fri, Oct 7, 2016 at 2:41 PM, FX <fxcoudert@gmail.com> wrote: >>>> Many POSIX systems have the bad habit of not restarting interrupted >>>> syscalls. On these systems it's up to the user to check for an error >>>> with errno == EINTR and restart manually. This patch does this for >>>> libgfortran, so that GFortran users don't have to do it. >>> >>> I have not much experience with EINTR, but is it garanteed that those EINTR loops will never cycle forever? >> >> Hmm, no I don't think so, but I don't think it'll be a problem. So on >> systems where syscalls are not restarted automatically, EINTR happens >> when the process receives a signal while blocked in a system call [1]. >> So I suppose in theory you could have a situation where something >> continuously fires signals at the process, and the result is some kind >> of race between the process restarting the syscall which then never >> manages to complete before being interrupted again. But I think this >> goes into the "Doctor, this hurts! Then don't do that" territory. >> >> There's some more info in https://www.python.org/dev/peps/pep-0475/ >> (Python nowadays does the same as this patch). > > > Just one concern (slightly different from the race you described) - > what if a user wants/expects a system call to be interrupted? With the > patch we would always restart the system call even if the user was > expecting it would be interrupted. For small calls like lseek this may > not be a big deal but if read on a pipe/socket/terminal is restarted > after being interrupted (e.g. with CTRL-C) we may loop forever even if > the user program was written to expect and handle EINTR after the read > call, e.g. to terminate nicely with "non async-safe" calls like printf > that couldn't be done in the handler. Concievable yes, but IMHO unlikely. And since many systems automatically restart syscalls, a program like the above perhaps isn't that robust to begin with? > This is discussed as "use case 2" in the PEP you referenced. Python > handles this case by explicitly calling user defined signal handlers > directly after EINTR and checking the return value from the handler, > only trying again if the handler reports success. Not so simple I > think with libgfortran. With GFortran, a problem is that due to the buffering, handling of record markers etc. there is no 1:1 mapping between Fortran READ/WRITE statements and read(2)/write(2) syscalls. So even if we let EINTR propagate all the way back up to the Fortran caller (as happens now, except for write()), it actually has no way of knowing what should be restarted.
On Fri, Oct 7, 2016 at 12:09 PM, Janne Blomqvist <blomqvist.janne@gmail.com> wrote: > On Fri, Oct 7, 2016 at 5:50 PM, Fritz Reese <fritzoreese@gmail.com> wrote: >> On Fri, Oct 7, 2016 at 8:59 AM, Janne Blomqvist >> <blomqvist.janne@gmail.com> wrote: >>> On Fri, Oct 7, 2016 at 2:41 PM, FX <fxcoudert@gmail.com> wrote: >>>>> Many POSIX systems have the bad habit of not restarting interrupted >>>>> syscalls. On these systems it's up to the user to check for an error >>>>> with errno == EINTR and restart manually. This patch does this for >>>>> libgfortran, so that GFortran users don't have to do it. >>>> >>>> I have not much experience with EINTR, but is it garanteed that those EINTR loops will never cycle forever? >>> >>> Hmm, no I don't think so, but I don't think it'll be a problem. So on >>> systems where syscalls are not restarted automatically, EINTR happens >>> when the process receives a signal while blocked in a system call [1]. >>> So I suppose in theory you could have a situation where something >>> continuously fires signals at the process, and the result is some kind >>> of race between the process restarting the syscall which then never >>> manages to complete before being interrupted again. But I think this >>> goes into the "Doctor, this hurts! Then don't do that" territory. >>> >>> There's some more info in https://www.python.org/dev/peps/pep-0475/ >>> (Python nowadays does the same as this patch). >> >> >> Just one concern (slightly different from the race you described) - >> what if a user wants/expects a system call to be interrupted? With the >> patch we would always restart the system call even if the user was >> expecting it would be interrupted. For small calls like lseek this may >> not be a big deal but if read on a pipe/socket/terminal is restarted >> after being interrupted (e.g. with CTRL-C) we may loop forever even if >> the user program was written to expect and handle EINTR after the read >> call, e.g. to terminate nicely with "non async-safe" calls like printf >> that couldn't be done in the handler. > > Concievable yes, but IMHO unlikely. And since many systems > automatically restart syscalls, a program like the above perhaps isn't > that robust to begin with? > >> This is discussed as "use case 2" in the PEP you referenced. Python >> handles this case by explicitly calling user defined signal handlers >> directly after EINTR and checking the return value from the handler, >> only trying again if the handler reports success. Not so simple I >> think with libgfortran. > > With GFortran, a problem is that due to the buffering, handling of > record markers etc. there is no 1:1 mapping between Fortran READ/WRITE > statements and read(2)/write(2) syscalls. So even if we let EINTR > propagate all the way back up to the Fortran caller (as happens now, > except for write()), it actually has no way of knowing what should be > restarted. > One issue that might need to be considered is whether the libgfortran routines would ever be indirectly called from code that is using fork()/exec(). We ran into a nasty regression in GNU Make 4.0/4.1 which was tickled when NLS support was enabled which indirectly pulled in the CoreFoundation framework and its threading support via libiconv. Upstream fixed this with... http://savannah.gnu.org/bugs/index.php?46261#comment12 http://git.savannah.gnu.org/cgit/make.git/commit/?id=85c788572d054bc2c41b84007875edbd37ad3ed5 in make 4.2. So using EINTR properly can be really tricky. > > > -- > Janne Blomqvist
On Oct 7, 2016, at 5:41 AM, FX <fxcoudert@gmail.com> wrote: > >> Many POSIX systems have the bad habit of not restarting interrupted >> syscalls. On these systems it's up to the user to check for an error >> with errno == EINTR and restart manually. This patch does this for >> libgfortran, so that GFortran users don't have to do it. > > I have not much experience with EINTR, but is it garanteed that those EINTR loops will never cycle forever? They will not. You get at most 1 for 1 signal, sometimes less. If you have a finite number of signals, you will have a finite number of loops.
On Oct 7, 2016, at 5:59 AM, Janne Blomqvist <blomqvist.janne@gmail.com> wrote: > > So I suppose in theory you could have a situation where something > continuously fires signals at the process, and the result is some kind > of race between the process restarting the syscall which then never > manages to complete before being interrupted again. But I think this > goes into the "Doctor, this hurts! Then don't do that" territory. No, this is in the territory of seriously broken software that I don't think has ever existed.
On Oct 7, 2016, at 7:50 AM, Fritz Reese <fritzoreese@gmail.com> wrote: > what if a user wants/expects a system call to be interrupted? Then it is interrupted. > With the patch we would always restart the system call even if No, this is a misunderstanding on your part. The signal is delivered and delivered first then the system call is restarted. > the user was expecting it would be interrupted. For small calls like lseek this may > not be a big deal but if read on a pipe/socket/terminal is restarted > after being interrupted (e.g. with CTRL-C) we may loop forever No, that isn't possible. > even if the user program was written to expect and handle EINTR after the read > call, e.g. to terminate nicely with "non async-safe" calls like printf > that couldn't be done in the handler. > > This is discussed as "use case 2" in the PEP you referenced. Python > handles this case by explicitly calling user defined signal handlers > directly after EINTR and checking the return value from the handler, > only trying again if the handler reports success. Not so simple I > think with libgfortran. Yes, it is. signals are delivered first. First in python, and first in C, there is a reason for that.
On Oct 7, 2016, at 9:42 AM, Jack Howarth <howarth.at.gcc@gmail.com> wrote: > > So using EINTR properly can be really tricky. I'd not phrase it that way. I'd phrase it as deferral can be tricky and choosing what action to do in a signal handler can be tricky. I don't mention deferral nor signal semantics, as these are significantly more challenging. Merely looping on EINTR I think is a no-brainer.
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c index 29818cd..43e33c8 100644 --- a/libgfortran/io/unix.c +++ b/libgfortran/io/unix.c @@ -298,8 +298,15 @@ static ssize_t raw_read (unix_stream * s, void * buf, ssize_t nbyte) { /* For read we can't do I/O in a loop like raw_write does, because - that will break applications that wait for interactive I/O. */ - return read (s->fd, buf, nbyte); + that will break applications that wait for interactive I/O. We + still can loop around EINTR, though. */ + while (true) + { + ssize_t trans = read (s->fd, buf, nbyte); + if (trans == -1 && errno == EINTR) + continue; + return trans; + } } static ssize_t @@ -316,7 +323,7 @@ raw_write (unix_stream * s, const void * buf, ssize_t nbyte) while (bytes_left > 0) { trans = write (s->fd, buf_st, bytes_left); - if (trans < 0) + if (trans == -1) { if (errno == EINTR) continue; @@ -333,22 +340,37 @@ raw_write (unix_stream * s, const void * buf, ssize_t nbyte) static gfc_offset raw_seek (unix_stream * s, gfc_offset offset, int whence) { - return lseek (s->fd, offset, whence); + while (true) + { + gfc_offset off = lseek (s->fd, offset, whence); + if (off == (gfc_offset) -1 && errno == EINTR) + continue; + return off; + } } static gfc_offset raw_tell (unix_stream * s) { - return lseek (s->fd, 0, SEEK_CUR); + while (true) + { + gfc_offset off = lseek (s->fd, 0, SEEK_CUR); + if (off == (gfc_offset) -1 && errno == EINTR) + continue; + return off; + } } static gfc_offset raw_size (unix_stream * s) { struct stat statbuf; - int ret = fstat (s->fd, &statbuf); - if (ret == -1) - return ret; + while (fstat (s->fd, &statbuf) == -1) + { + if (errno == EINTR) + continue; + return -1; + } if (S_ISREG (statbuf.st_mode)) return statbuf.st_size; else @@ -390,7 +412,13 @@ raw_truncate (unix_stream * s, gfc_offset length) lseek (s->fd, cur, SEEK_SET); return -1; #elif defined HAVE_FTRUNCATE - return ftruncate (s->fd, length); + while (ftruncate (s->fd, length) == -1) + { + if (errno == EINTR) + continue; + return -1; + } + return 0; #elif defined HAVE_CHSIZE return chsize (s->fd, length); #else @@ -409,7 +437,17 @@ raw_close (unix_stream * s) else if (s->fd != STDOUT_FILENO && s->fd != STDERR_FILENO && s->fd != STDIN_FILENO) - retval = close (s->fd); + { + retval = close (s->fd); + /* close() and EINTR is special, as the file descriptor is + deallocated before doing anything that might cause the + operation to be interrupted. Thus if we get EINTR the best we + can do is ignore it and continue (otherwise if we try again + the file descriptor may have been allocated again to some + other file). */ + if (retval == -1 && errno == EINTR) + retval = errno = 0; + } else retval = 0; free (s); @@ -463,7 +501,7 @@ buf_flush (unix_stream * s) return 0; if (s->physical_offset != s->buffer_offset - && lseek (s->fd, s->buffer_offset, SEEK_SET) < 0) + && raw_seek (s, s->buffer_offset, SEEK_SET) < 0) return -1; writelen = raw_write (s, s->buffer, s->ndirty); @@ -518,7 +556,7 @@ buf_read (unix_stream * s, void * buf, ssize_t nbyte) to_read = nbyte - nread; new_logical = s->logical_offset + nread; if (s->physical_offset != new_logical - && lseek (s->fd, new_logical, SEEK_SET) < 0) + && raw_seek (s, new_logical, SEEK_SET) < 0) return -1; s->buffer_offset = s->physical_offset = new_logical; if (to_read <= BUFFER_SIZE/2) @@ -587,7 +625,7 @@ buf_write (unix_stream * s, const void * buf, ssize_t nbyte) { if (s->physical_offset != s->logical_offset) { - if (lseek (s->fd, s->logical_offset, SEEK_SET) < 0) + if (raw_seek (s, s->logical_offset, SEEK_SET) < 0) return -1; s->physical_offset = s->logical_offset; }