Skip to content

gh-149474: use Py_fopen in Binary{Reader,Writer} for audit hook and path-like support#149524

Merged
pablogsal merged 4 commits intopython:mainfrom
maurycy:remote-debugger-fopen
May 9, 2026
Merged

gh-149474: use Py_fopen in Binary{Reader,Writer} for audit hook and path-like support#149524
pablogsal merged 4 commits intopython:mainfrom
maurycy:remote-debugger-fopen

Conversation

@maurycy
Copy link
Copy Markdown
Contributor

@maurycy maurycy commented May 8, 2026 •
edited
Loading

As a bonus: PEP 446... so that's a hat-trick: 519 + 578 + 446!

Closes #149474

@maurycy maurycy requested a review from pablogsal as a code owner May 8, 2026 00:24
@bedevere-app bedevere-app Bot mentioned this pull request May 8, 2026
Closed
maurycy
maurycy commented May 8, 2026
View reviewed changes
Comment thread Modules/_remote_debugging/binary_io_reader.c
maurycy
maurycy commented May 8, 2026
View reviewed changes
Comment thread Modules/_remote_debugging/binary_io_writer.c
@maurycy
Copy link
Copy Markdown
Contributor Author

maurycy commented May 8, 2026

cc @ZeroIntensity @sobolevn (you +1 my comment)

sobolevn
sobolevn reviewed May 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment thread Modules/_remote_debugging/binary_io_reader.c
/* Add file descriptor-level hints for better kernel I/O scheduling */
#if defined(__linux__) && defined(POSIX_FADV_SEQUENTIAL)
(void)posix_fadvise(reader->fd, 0, 0, POSIX_FADV_SEQUENTIAL);
(void)posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL);
Copy link
Copy Markdown
Member

@sobolevn sobolevn May 8, 2026

Choose a reason for hiding this comment

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

Question: os_posix_fadvise_impl has two implementation details that this place does not: Py_BEGIN_ALLOW_THREADS is set and async_err = PyErr_CheckSignals() is checked.

Do we need to do this here?

Copy link
Copy Markdown
Member

@pablogsal pablogsal May 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I don’t think we need it here. These calls are best-effort performance hints and intentionally ignore all posix_fadvise() failures, unlike os.posix_fadvise where the result is user-visible and must handle EINTR/signals correctly. PyErr_CheckSignals() would make an advisory hint observable by turning some interruptions into failures, which does not match the surrounding madvise()/posix_fadvise() handling.

y_BEGIN_ALLOW_THREADS also is not needed for correctness here; if we wanted to tune this path, we would probably need to look at the whole block, since mmap(... MAP_POPULATE) and madvise(... MADV_WILLNEED) are also currently called with the GIL held.

Comment thread Modules/_remote_debugging/binary_io_writer.c

if (writer->fp) {
fclose(writer->fp);
Py_fclose(writer->fp);
Copy link
Copy Markdown
Member

@sobolevn sobolevn May 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

question: why don't we check the return code here? Because the returned type is void?

Copy link
Copy Markdown
Member

@pablogsal pablogsal May 8, 2026

Choose a reason for hiding this comment

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

There is nothing we can do if close fails, we could log perhaps...

Copy link
Copy Markdown
Member

@picnixz picnixz May 9, 2026

Choose a reason for hiding this comment

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

In this case I woudl suggest that we explicitly suppress the returned value and add a comment. We can also raise an unraisable exception (which I do in curses)

pablogsal
pablogsal approved these changes May 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@pablogsal pablogsal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Flawless! I cannot find anything fundamentally wrong here. Amazing job!

@pablogsal pablogsal merged commit 354ef33 into python:main May 9, 2026
59 checks passed
@maurycy maurycy deleted the remote-debugger-fopen branch May 9, 2026 00:01
@pablogsal pablogsal added the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label May 9, 2026
@miss-islington-app
Copy link
Copy Markdown

miss-islington-app Bot commented May 9, 2026

Thanks @maurycy for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15.
🐍🍒⛏🤖

@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 9, 2026

GH-149586 is a backport of this pull request to the 3.15 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label May 9, 2026
pablogsal pushed a commit that referenced this pull request May 9, 2026
@miss-islington @maurycy
[3.15] gh-149474: use Py_fopen in Binary{Reader,Writer} for audit…
2b1eed4 
… hook and path-like support (GH-149524) (#149586)

gh-149474: use `Py_fopen` in `Binary{Reader,Writer}` for audit hook and path-like support (GH-149524)
(cherry picked from commit 354ef33)

Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevn sobolevn sobolevn left review comments

@picnixz picnixz picnixz left review comments

@pablogsal pablogsal pablogsal approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

_remote_debugging.BinaryWriter() bypasses file-open audit hooks (PEP 578)

4 participants

@maurycy @sobolevn @picnixz @pablogsal