Skip to content

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

Merged
vstinner merged 3 commits into
python:2.7from
nirs:fileio-gil-2.7
Dec 7, 2017
Merged

[2.7] bpo-32186: Release the GIL during fstat and lseek calls#4651
vstinner merged 3 commits into
python:2.7from
nirs:fileio-gil-2.7

Conversation

@nirs

@nirs nirs commented Nov 30, 2017

Copy link
Copy Markdown
Contributor

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

nirs commented Nov 30, 2017

Copy link
Copy Markdown
Contributor Author

@vstinner can you review?

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.
@nirs nirs changed the title bpo-XXX: Release the GIL during fstat and lseek calls bpo-32186: Release the GIL during fstat and lseek calls Nov 30, 2017
@vstinner vstinner changed the title bpo-32186: Release the GIL during fstat and lseek calls [2.7] bpo-32186: Release the GIL during fstat and lseek calls Nov 30, 2017
@vstinner

Copy link
Copy Markdown
Member

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

Comment thread Modules/_io/fileio.c

Py_BEGIN_ALLOW_THREADS
res = fstat(self->fd, &st);
Py_END_ALLOW_THREADS

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gst Dec 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Modules/_io/fileio.c

Py_BEGIN_ALLOW_THREADS
res = fstat(self->fd, &st);
Py_END_ALLOW_THREADS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

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

vstinner commented Dec 4, 2017

Copy link
Copy Markdown
Member

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

@nirs

nirs commented Dec 4, 2017

Copy link
Copy Markdown
Contributor Author

@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

vstinner commented Dec 4, 2017

Copy link
Copy Markdown
Member

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

nirs commented Dec 5, 2017

Copy link
Copy Markdown
Contributor Author

@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

vstinner commented Dec 5, 2017

Copy link
Copy Markdown
Member

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

nirs commented Dec 5, 2017

Copy link
Copy Markdown
Contributor Author

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

@vstinner

vstinner commented Dec 5, 2017

Copy link
Copy Markdown
Member

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

Thanks.

Same issue seen in fileio exists also in fileobject, fixed in the same
way.
@nirs

nirs commented Dec 7, 2017

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@vstinner vstinner merged commit 830daae into python:2.7 Dec 7, 2017
@vstinner

vstinner commented Dec 7, 2017

Copy link
Copy Markdown
Member

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

@nirs nirs deleted the fileio-gil-2.7 branch October 1, 2022 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants