bpo-19675: Fixes pool cleanup issue

[dsoprea]

http://bugs.python.org/issue19675

• This is my first contribution. Please forgive if the commit-message is not formatted correctly. I was not able to find the guidelines for the convention. I read "https://docs.python.org/devguide/committing.html" and "https://docs.python.org/devguide/pullrequest.html".
• I'm assuming this is too trivial a change to make a change to Misc/ACKS.

"make test" output:

28 tests skipped:
    test_bz2 test_ctypes test_curses test_dbm_gnu test_dbm_ndbm
    test_devpoll test_gzip test_idle test_kqueue test_lzma test_msilib
    test_ossaudiodev test_readline test_smtpnet test_sqlite test_ssl
    test_startfile test_tcl test_tix test_tk test_ttk_guionly
    test_ttk_textonly test_turtle test_winconsoleio test_winreg
    test_winsound test_zipfile64 test_zlib

Total duration: 4 min 18 sec
Tests result: SUCCESS

"make patchcheck" also succeeds. Not sure if any other checks are required prior to posting the PR and relying on any automatic checks that ensue there.

This would obviously also need to be labeled for backporting.

https://bugs.python.org/issue19675

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

1. If you don't have an account on b.p.o, please create one
2. Make sure your GitHub username is listed in "Your Details" at b.p.o
3. If you have not already done so, please sign the PSF contributor agreement
4. If you just signed the CLA, please wait at least a day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
5. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

[dsoprea]

Waiting on CLA approval.

[briancurtin]

bpo-19675: Fix pool cleanup issue would be the ideal commit message/title of the PR, you just needed to prefix bpo- for it to be good :)

[briancurtin]

We should also have a test here to prove that this new functionality works out alright.

[briancurtin]

This should probably be util.debug, right?

[applio]

Per continued discussion on https://bugs.python.org/issue19675 this patch still also needs tests. If you want to close this PR and submit another or continue iterating here, I'll leave that to you.

[dsoprea]

I'll keep iterating. I've accounted for the other suggestions locally. I'm just trying to figure out what needs to be done to test this. See other thread.

…

On Mon, Feb 13, 2017 at 11:56 AM, Davin Potts ***@***.***> wrote: Per continued discussion on https://bugs.python.org/issue19675 this patch still also needs tests. If you want to close this PR and submit another or continue iterating here, I'll leave that to you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#57 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArraj3dH4b39wB6-5d4mwE4L3s1MPqKks5rcItPgaJpZM4L-qfA> .

[dsoprea]

I just added the following note about testing to the issue:

(http://bugs.python.org/issue19675)

I don't think this can be tested. Throwing exceptions in the remote process causes exceptions that can't be caught in the same way (when the initializer fails the pool just attempts to recreate the process over and over) and I don't think it'd be acceptable to try to spawn too many processes in order to induce the original problem. There's not a lot of surface area to what we're doing here. We can't simulate failures any other way.

Once we get to the point where we can merge, I'll close this PR, cherry-pick the commit into the version-specific branches, and create new PRs.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@c33ee85). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master      #57   +/-   ##
=========================================
  Coverage          ?   82.37%           
=========================================
  Files             ?     1427           
  Lines             ?   350958           
  Branches          ?        0           
=========================================
  Hits              ?   289088           
  Misses            ?    61870           
  Partials          ?        0

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c33ee85...007f134. Read the comment docs.

[smcolby]

@applio @briancurtin I am currently facing the limitations that would be resolved by this PR and would like to help in any way possible to get this merged. Do either of you have recommendations as to how we can get this PR passing all tests?

[JulienPalard]

I'm, not fan of the actual patch: if an exception happen during _handle_workers, in a case it needs to respawn just one worker, you'll terminate all correctly running workers, which look bad.

[brettcannon]

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

[encukou]

@brettcannon, are you aware of your duplicate comments?

[brettcannon]

@encukou yep, this one issue triggered an odd bug in my script; should be the only case.

[JulienPalard]

Merged #5614 so this should be fixed now.