GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-130209: Retry path-based os functions on EINTR by larryhastings · Pull Request #160544 · python/cpython Conversation
Contributor
Extends PEP 475's retry-on- BackgroundPEP 475 (3.5) added
The original report came from a program with a
Changes
Behavior on retry matches PEP 475 exactly: on The code is mostly mechanical cookie-cutter code as follows, adjusted to conform to function-specific details: for (;;) { Py_BEGIN_ALLOW_THREADS result = syscall(...); Py_END_ALLOW_THREADS // interrupted by a signal (EINTR)? retry! if ((result != 0) && (errno == EINTR)) { if (PyErr_CheckSignals()) { return NULL; } continue; } break; } Two pre-existing bugs fixed in passing, in code that had to be restructured anyway:
Testing
Not compiled/tested locally: AI InvolvementI wrote the code, Claude Fable 5.1 wrote the tests and I reviewed them. Issue: gh-130209
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsPEP 475 (3.5) added "EINTR" retry loops to the functions expected to block:
read, write, open, stat, select, sockets, wait*, and so on. The path-based
functions (stat, rename, utime, unlink, listdir, ...) were skipped,
on the assumption that they don't block long enough to be interrupted.
That assumption doesn't hold on network or userspace filesystems.
This commit adds retry loops to thirty more functions, to fill in the gap:
* os.access
* os.chdir
* os.chflags
* os.chmod
* os.chown
* os.chroot
* os.getxattr
* os.lchflags
* os.lchmod
* os.lchown
* os.link
* os.listdir
* os.listxattr
* os.lstat
* os.mkdir
* os.readlink
* os.removexattr
* os.rename
* os.replace
* os.rmdir
* os.scandir (open, iteration, and DirEntry.stat() / is_*() )
* os.setxattr
* os.stat
* os.statvfs
* os.statx
* os.symlink
* os.truncate
* os.unlink
* os.utime
* os._fcopyfile
os.access() is a special case worth calling out: it reports failure by
return value, so before this change an EINTR made it silently return
False for a perfectly readable file--with no exception at all.
Issue: pythongh-130209
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
encukou
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main issue I see is handling non-idempotent operations (like mkdir, rmdir, rename, readdir, unlink, symlink, fcopyfile, chdir, chroot, removexattr; setattr with Can you add the tests? We already test some stuff that needs root. Otherwise, I have a few comments, and nits (ignore those if your choices are deliberate).
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Comment on lines
+1
to
+10
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems too long for a news entry; consider:
Suggested change
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This line might be after
Suggested change
(same in
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing code uses PEP-7 style; please don't change it.
Suggested change
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Result checking is understandably inconsistent between functions. But within functions, consider making it textually identical -- either:
Suggested change
or change to Comparing
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use the same condition within a function.
Suggested change
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Comment on lines
+6910
to
+6922
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Moving the #ifdef HAVE_UNLINKAT if (dir_fd != DEFAULT_DIR_FD) { if (HAVE_UNLINKAT_RUNTIME) { Py_BEGIN_ALLOW_THREADS result = unlinkat(dir_fd, path->narrow, 0); Py_END_ALLOW_THREADS } else { argument_unavailable_error(NULL, "dir_fd"); return NULL; } } else #endif /* HAVE_UNLINKAT */ { Py_BEGIN_ALLOW_THREADS result = unlink(path->narrow); Py_END_ALLOW_THREADS } (There's a bunch of cases like this in other functions.)
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use the same condition.
Suggested change
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(Hopefully no one reads directories in a signal handler, but better be safe.) AFAICS, that means means the outer
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Comment on lines
+17604
to
+17605
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not the only thing that happens, though. There's also a call to
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
encukou
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(And these should technically get their own
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
ReviewersUh oh!
There was an error while loading. Please reload this page. AssigneesLabels
awaiting core review
extension-modules
ProjectsMilestoneNo milestoneDevelopmentSuccessfully merging this pull request may close these issues. Uh oh!
There was an error while loading. Please reload this page. 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. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| You are visitor 01402503 — GeezerVison does not store the pages you look up. Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required. |