Skip to content

Conversation

@rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Oct 22, 2024

for example:

import multiprocessing


def f():
    print('hello')
    print('world')

if __name__ == "__main__":
    p = multiprocessing.Process(target=f)
    p.start()
    p.join()

    # In a more context, it's closed
    p.close()
    
    # Currnet context
    # It's thrown a ValueError
    if p.exitcode == 0:
        pass

    # Changed
    if not p.is_closed():
        print(p.exitcode)

@rruuaanng
Copy link
Contributor Author

It seems that this NEWS error can be ignored.

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

More generally, required CI checks must never be ignored except for NEWS missing / issue missing since normal users cannot apply the skip news/issue labels themselves.

@erlend-aasland erlend-aasland changed the title gh-87063: Add is_closed method to multiprocessing.Process gh-87063: Add is_closed attribute to multiprocessing.Process Oct 25, 2024
@erlend-aasland erlend-aasland changed the title gh-87063: Add is_closed attribute to multiprocessing.Process gh-87063: Add closed attribute to multiprocessing.Process Oct 25, 2024
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

PR author did not listen to feedback. Requesting changes to make this point clearer.

@bedevere-app
Copy link

bedevere-app bot commented Oct 26, 2024

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.

@rruuaanng
Copy link
Contributor Author

Sorry for my misunderstanding. I thought it was to change NEWS to what's NEWS, but it was actually to add to it. I'm sorry for that. I've made change :)

@rruuaanng
Copy link
Contributor Author

Thanks to Bénédikt for taking responsibility for this PR. And, thanks to erlend for pointing out my mistake.👍

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

A final nitpick. Concerning the naming of the attribute, I suggested a property + closed but if @gpshead feels that it should be a method + is_closed(), then I'm sorry in advance :) (just recalling that my rationale was that multiprocessing.connection.Connection uses the closed property but I don't know if properties are preferred over true methods when we are just returning a private attribute).

@rruuaanng

This comment was marked as outdated.

@rruuaanng
Copy link
Contributor Author

@erlend-aasland Please review them again! :)

@erlend-aasland
Copy link
Contributor

@erlend-aasland Please review them again! :)

I asked some questions on the issue; IMO, they should be clarified before eventually landing this PR. In other words: I believe more discussion and perhaps a larger audience is needed.

@rruuaanng
Copy link
Contributor Author

Happy New Year, everyone. It's been a long time since the last review. Do we still need to keep this open?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants