◐ Shell
reader mode source ↗
Skip to content

bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn().#6332

Merged
serhiy-storchaka merged 11 commits into
python:masterfrom
serhiy-storchaka:posix_spawn
May 1, 2018
Merged

bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn().#6332
serhiy-storchaka merged 11 commits into
python:masterfrom
serhiy-storchaka:posix_spawn

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Apr 1, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@pablogsal

pablogsal commented Apr 1, 2018

Copy link
Copy Markdown
Member

Should I close #6331 in favour of this one?

gpshead
gpshead requested changes Apr 1, 2018
View reviewed changes

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

Hide comment

I made a couple of commits to fixup trivial issues.

@bedevere-bot

bedevere-bot commented Apr 1, 2018

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@gpshead gpshead assigned gpshead and unassigned gpshead Apr 1, 2018
@gpshead gpshead mentioned this pull request Apr 1, 2018
Closed
zware
zware reviewed Apr 10, 2018
View reviewed changes

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

Hide comment

Added my $0.02USD on the blurb contents below, and @gpshead's suggestions look sound to me as well. Otherwise, this looks like a nice cleanup.

serhiy-storchaka
serhiy-storchaka commented Apr 10, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hide comment

Thank you for fixing these issues and for documenting the new function @gpshead.

But increfing a borrowed reference is neccessary, and supporting other sequences as argv can open a can of worms. It should be discussed separately if there is a need of this feature.

@serhiy-storchaka

serhiy-storchaka commented Apr 10, 2018

Copy link
Copy Markdown
Member Author

Added new tests (mainly inspired by tests for the third-party module https://github.com/projectcalico/posix_spawn) and improved the documentation.

@vstinner

vstinner commented Apr 26, 2018

Copy link
Copy Markdown
Member

@gpshead: @serhiy-storchaka updated his PR and replied to your comments, would you mind to review it again?

@serhiy-storchaka: Travis CI is unhappy. Did you check why?

This PR fixes a reference leak which makes the Linux Refleaks buildbot red: https://bugs.python.org/issue33357

cc @pablogsal

Note: I didn't review the PR.

@serhiy-storchaka serhiy-storchaka changed the title bpo-20104: Fix numerous issues with os.posix_spawn(). Apr 26, 2018
@serhiy-storchaka serhiy-storchaka merged commit ef34753 into python:master May 1, 2018
@bedevere-bot

bedevere-bot commented May 1, 2018

Copy link
Copy Markdown

@serhiy-storchaka: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington

miss-islington commented May 1, 2018

Copy link
Copy Markdown
Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the posix_spawn branch May 1, 2018 13:45
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 1, 2018
@serhiy-storchaka @miss-islington
bpo-20104: Improve error handling and fix a reference leak in os.posi… …
e1b4bff 
…x_spawn(). (pythonGH-6332)

(cherry picked from commit ef34753)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot

bedevere-bot commented May 1, 2018

Copy link
Copy Markdown

GH-6673 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request May 1, 2018
@miss-islington @serhiy-storchaka
bpo-20104: Improve error handling and fix a reference leak in os.posi… …
77fa783 
…x_spawn(). (GH-6332)

(cherry picked from commit ef34753)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zware zware zware left review comments

@gpshead gpshead gpshead requested changes

Assignees

No one assigned

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@serhiy-storchaka @pablogsal @bedevere-bot @vstinner @miss-islington @gpshead @zware @the-knights-who-say-ni