[2.7] bpo-32186: Release the GIL during fstat and lseek calls

[nirs]

In fileio, there were 3 fstat() calls and one lseek() call that did not
release the GIL during the call. This can cause all threads to hang for
unlimited time when using io.FileIO with inaccessible NFS server.

https://bugs.python.org/issue32186

[nirs]

@vstinner can you review?

[vstinner]

I prefer to wait until PR #4652 is merged before reviewing the backport to Python 2.7.

[gst]

Hi,

wouldn't it be slightly better to make the 2 sys calls in the same ALLOW_THREADS block, given they follow each other in the usual case ?

like this:

Py_BEGIN_ALLOW_THREADS
res = fstat(self->fd, &st);
if (res == 0) {
    pos = lseek(self->fd, 0L, SEEK_CUR);
}
Py_END_ALLOW_THREADS
if (res == 0) {
    end = st.st_size;
    # etc..

regards,

gst.

[nirs]

Thanks for the review @gst. The code is less clear this way, and we are not calling this in a tight loop, so I don't think this optimization is needed.

[gst]

agreed.
but now I see that the lseek call isn't checked for error (return value == -1)..
shouldn't it be actually ?

EDIT: well, it falls back to the returned default value in such case, so guess it's good enough..

[nirs]

I think the current error handling is fragile. But I want to make the minimal change needed for this issue.

[vstinner]

The best would be to backport _Py_fstat() from master: it returns the file size properly on Windows by using GetFileInformationByHandle(). Be honestly, I don't think that it's worth it and it's perfectly fine to release/acquire the GIL twice in this short function.

[vstinner]

The builtin "file" type has the same bug (doesn't release the GIL) but isn't fixed by this change.

Please update also file_read() in Objects/fileobject.c.

[vstinner]

The best would be to backport _Py_fstat() from master: it returns the file size properly on Windows by using GetFileInformationByHandle(). Be honestly, I don't think that it's worth it and it's perfectly fine to release/acquire the GIL twice in this short function.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[vstinner]

I checked Modules/_io/fileio.c: with your PR, it seems like the GIL is now released for all "blocking" syscalls.

[nirs]

@vstinner in 2.7 file_read() in Objects/fileobject.c:

1088         FILE_BEGIN_ALLOW_THREADS(f)
1089         errno = 0;                                                                                                                                                                       
1090         chunksize = Py_UniversalNewlineFread(BUF(v) + bytesread,
1091                   buffersize - bytesread, f->f_fp, (PyObject *)f);
1092         interrupted = ferror(f->f_fp) && errno == EINTR;
1093         FILE_END_ALLOW_THREADS(f)

What do you suggest to change?

[vstinner]

What do you suggest to change?

In Python 2.7, dircheck() calls fstat() and new_buffersize() calls fstat() and lseek() without releasing the GIL. I was talking about these functions.

[nirs]

@vstinner, you requested changes in the patch in Objects/fileobj.c - but the code looks correct to me. Can you explain what is the issue and how do you think it should be fixed?

[vstinner]

Python 2.7, see my " <~~~~ GIL not released":

static PyFileObject*
dircheck(PyFileObject* f)
{
#if defined(HAVE_FSTAT) && defined(S_IFDIR) && defined(EISDIR)
    struct stat buf;
    if (f->f_fp == NULL)
        return f;
    if (fstat(fileno(f->f_fp), &buf) == 0 && <~~~~ GIL not released
        S_ISDIR(buf.st_mode)) {
        char *msg = strerror(EISDIR);
        PyObject *exc = PyObject_CallFunction(PyExc_IOError, "(isO)",
                                              EISDIR, msg, f->f_name);
        PyErr_SetObject(PyExc_IOError, exc);
        Py_XDECREF(exc);
        return NULL;
    }
#endif
    return f;
}

static size_t
new_buffersize(PyFileObject *f, size_t currentsize)
{
#ifdef HAVE_FSTAT
    off_t pos, end;
    struct stat st;
    if (fstat(fileno(f->f_fp), &st) == 0) { <~~~~ GIL not released
        end = st.st_size;
        /* The following is not a bug: we really need to call lseek()
           *and* ftell().  The reason is that some stdio libraries
           mistakenly flush their buffer when ftell() is called and
           the lseek() call it makes fails, thereby throwing away
           data that cannot be recovered in any way.  To avoid this,
           we first test lseek(), and only call ftell() if lseek()
           works.  We can't use the lseek() value either, because we
           need to take the amount of buffered data into account.
           (Yet another reason why stdio stinks. :-) */
        pos = lseek(fileno(f->f_fp), 0L, SEEK_CUR); <~~~~ GIL not released
        if (pos >= 0) {
            pos = ftell(f->f_fp);
        }
        if (pos < 0)
            clearerr(f->f_fp);
        if (end > pos && pos >= 0)
            return currentsize + end - pos + 1;
        /* Add 1 so if the file were to grow we'd notice. */
    }
#endif
    /* Expand the buffer by an amount proportional to the current size,
       giving us amortized linear-time behavior. Use a less-than-double
       growth factor to avoid excessive allocation. */
    return currentsize + (currentsize >> 3) + 6;
}

[nirs]

@vstinner thanks! what a horrible code duplication :-) I'll add a minimal fix in the next commit.

[vstinner]

I'll add a minimal fix in the next commit.

Thanks.

[nirs]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

[vstinner]

Thank you @nirs for the update, the PR is now complete ;-) I merged it.