bpo-32608: Adding a server into socketserver that handles client connection in new "multiprocessing.Process" processes. #5258

deliberist · 2018-01-21T13:01:55Z

https://bugs.python.org/issue32608

…e passed into sub-processes created by the multiprocessing package.

the-knights-who-say-ni · 2018-01-21T13:01:58Z

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 our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

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

deliberist · 2018-01-21T13:17:01Z

I signed the CLA yesterday, does it take a few days for that to be seen?

pppery · 2018-01-21T22:36:18Z

Doc/library/socketserver.rst

-The :class:`ForkingMixIn` class is used in the same way, except that the server
-will spawn a new process for each request.
+The :class:`ForkingMixIn` and :class:`ProcessingMixIn` classes can be used in
+the same way, except that the server will spawn a new process for each request.
 Available only on POSIX platforms that support :func:`~os.fork`.


Presumably ProcessingMixIn can be used on Windows, since multiprocessing.Process can

Correct, that was my intent.

Since the multiprocessing.Process object is more portable, it makes sense to potentially use that if a developer wanted to. I didn't think removing the existing ForkingMixIn class would be productive since that would break anything that uses it, and there is probably still good use cases to use a lighter weight "os.fork()" implementation on POSIX systems.

What I had meant to say there was that the documentation was incorrect by implying that ProcessingMixIn was posix-only

I think I understand now, it wasn't necessarily documentation that I changed but more documentation that I didn't change. I'm updating the rst file now.

If interested, the specific comment for this comment is:
15ec16b

…ggesting ProcessingMixIn (and associated server classes) can still be used on non-POSIX systems.

…t/Library.

…after each test, which caused the tests to fail.

deliberist · 2018-02-10T18:49:25Z

For my own sanity, I'm dropping some useful commands I used when building/testing these changes.

Linux builds:

cd cpython
./configure --with-pydebug
make -j
./python -m test.pythoninfo
./python -m test
./python -m test -v test.test_socketserver -uall

Windows builds:

cd cpython
.\PCbuild\clean.bat
.\PCbuild\build.bat -e
.\PCbuild\win32\python.exe -m test.pythoninfo
.\PCbuild\win32\python.exe -m test
.\PCbuild\win32\python.exe -m test -v test.test_socketserver -uall
.\PCbuild\rt.bat -q -uall -u-cpu -rwW --slowest --timeout=1200 --fail-env-changed -j0

deliberist · 2018-02-10T19:49:02Z

Can someone with more Python on Windows experience be able to explain to me why the AppVeyor build is failing?

To me, it looks like the AppVeyor is failing on the test_threading module (which I did not modify) and the test_socketserver (which I did modify). However, I do not understand the circumstances where the test is failing on Windows but not on my LinuxMint 18.1 dev machine.

I would like this pull-request to get merged as soon as we can, but I doubt it will make it much farther with one of the build checks failing.

-- Thanks

pitrou

Thanks for posting this PR. I posted some comments below.
Also, it seems the tests fail on Windows (see the AppVeyor CI failure).

pitrou · 2018-05-01T21:15:46Z

Doc/library/socketserver.rst

@@ -99,11 +99,12 @@ server classes.


 .. class:: ForkingMixIn
+           ProcessingMixIn


You need to add a versionadded marker for the new class.

pitrou · 2018-05-01T21:16:02Z

Doc/library/socketserver.rst

   :class:`ForkingMixIn` and the Forking classes mentioned below are
   only available on POSIX platforms that support :func:`~os.fork`.

 .. class:: ForkingTCPServer
           ForkingUDPServer
+           ProcessingTCPServer


Ditto here (need a versionadded marker).

pitrou · 2018-05-01T21:16:48Z

Doc/whatsnew/3.7.rst

@@ -969,6 +969,13 @@ Changes in the Python API
  work as expected on all platforms.
  (Contributed by Yury Selivanov in :issue:`32331`.)

+* New :mod:`socketserver` classes were added called


This snippet will have to go into 3.8.rst instead. (also, make sure to add a "Contributed by..." note in parentheses as in the other entries).

pitrou · 2018-05-01T21:19:45Z

Lib/socketserver.py

+        # above max_children.
+        while len(self.active_children) >= self.max_children:
+            try:
+                pid, _ = os.waitpid(-1, 0)


I don't think using waitpid is ok with the multiprocessing version. Instead, use either one of the Process methods (perhaps join()) or the multiprocessing.connection.wait function if you need to wait on multiple objects (see https://docs.python.org/3/library/multiprocessing.html#multiprocessing.connection.wait ).

pitrou · 2018-05-01T21:20:22Z

Lib/test/test_socketserver.py

+        # The ProcessingTCPServer and ProcessingUDPServer both modify the
+        # environment, specifically the "multiprocessing.process._dangling"
+        # WeakSet() of created processes.
+        self._multiproc_dangling = multiprocessing.process._dangling.copy()


This sounds like a bug. Perhaps you need to call Process.join at some point.

Thinking about this some more, I think you're right. I've refactored the logic a little bit so that the tests do not need to clean up dangling processes.

bedevere-bot · 2018-05-01T21:22:57Z

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.

…multiprocessing # Conflicts: # Doc/whatsnew/3.7.rst

…multiprocessing

…les.

…ess.join() is called, rather than os.waitpid()

…ngling" references; and fixed some test_socketserver tests.

…ent.

deliberist · 2018-05-06T21:20:27Z

Hi Antoine (pitrou). I've made the changes you suggested. However the CI builds fail on Windows because of dangling threads that I cannot seem to track down why. Do you have a few minutes have a second set of eyes looking at the errors?

Locally, I print a traceback in threading.py when a new thread is added to the threading._dangling WeakSet. This tells me where the code is when it creates the threads that are still dangling at the end of the test_socketserver tests.

I added this into threading.py, in Thread.__init__():

        import sys
        import traceback
        print()
        print('\tNAME = {}'.format(self._name))
        traceback.print_stack(file=sys.stdout)

This bug is close to being complete, I cannot figure out why the Process objects are creating these extra threads that are not being cleaned up.

-- Thanks!

…server_and_multiprocessing # Conflicts: # Doc/whatsnew/3.8.rst # Lib/socketserver.py

deliberist · 2018-06-07T21:36:16Z

I have looked at the code over and over again. I keep coming to the conclusion that the reason the AppVeyor tests are failing only on Windows is because there is a problem with the way Windows handles multiple processes. For some reason, on Windows there is a dangling thread laying around after the unit tests run. I just cannot find exactly where and why a thread gets created but never cleaned up in the Thread._dangling object.

This scenario does not happen on Linux systems, which leads me to believe this problem is a bit out of scope of this ticket.

Can someone look and confirm? I'd really like to close this ticket so it can be merged into the main baseline, however the failing tests on Windows seems to be preventing this ticket from advancing.

vstinner · 2018-06-08T08:04:38Z

AppVeyor spotted a dangling thread:

0:03:19 [286/416/2] test_socketserver failed (env changed)
test_forking_handled (test.test_socketserver.ErrorHandlerTest) ... skipped 'requires forking'
test_forking_not_handled (test.test_socketserver.ErrorHandlerTest) ... skipped 'requires forking'
test_sync_handled (test.test_socketserver.ErrorHandlerTest) ... ok
test_sync_not_handled (test.test_socketserver.ErrorHandlerTest) ... ok
test_threading_handled (test.test_socketserver.ErrorHandlerTest) ... ok
test_threading_not_handled (test.test_socketserver.ErrorHandlerTest) ... ok
test_all (test.test_socketserver.MiscTestCase) ... ok
test_shutdown_request_called_if_verify_request_false (test.test_socketserver.MiscTestCase) ... ok
test_ForkingTCPServer (test.test_socketserver.SocketServerTest) ... skipped 'requires forking'
test_ForkingUDPServer (test.test_socketserver.SocketServerTest) ... skipped 'requires forking'
test_ForkingUnixDatagramServer (test.test_socketserver.SocketServerTest) ... skipped 'requires Unix sockets'
test_ForkingUnixStreamServer (test.test_socketserver.SocketServerTest) ... skipped 'requires Unix sockets'
test_ProcessingTCPServer (test.test_socketserver.SocketServerTest) ... creating server
ADDR = ('127.0.0.1', 4730)
CLASS = <class 'socketserver.ProcessingTCPServer'>
server running
test client 0
test client 1
test client 2
waiting for server
done
Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)
Dangling thread: <Thread(Thread-3, started daemon 2932)>
Dangling thread: <_MainThread(MainThread, started 3804)>
ok
(...)
Warning -- threading._dangling was modified by test_socketserver
  Before: <_weakrefset.WeakSet object at 0x03330ED0>
  After:  <_weakrefset.WeakSet object at 0x03330E50>

1 test altered the execution environment:
    test_socketserver

deliberist · 2018-06-08T09:17:53Z

@vstinner, Yup I see that there is a dangling thread. I cannot see what is different when running on Windows that produces the dangling thread that does not happen on Linux.

Updating fork with that of the upstream.

Updating socketserver_and_multiprocessing with that of master.

github-actions · 2022-08-14T00:12:29Z

This PR is stale because it has been open for 30 days with no activity.

deliberist · 2022-08-14T14:31:49Z

FWIW, this could still be merged in, I just never had the time or experience to figure out why the Windows CI tests failed. I've tested the branch on Windows directly and it passed. I do not understand why the CI was failing, but directly on a Windows host passed.

github-actions · 2023-02-27T00:11:48Z

This PR is stale because it has been open for 30 days with no activity.

Adding a server into socketserver that allows client connections to b…

da98359

…e passed into sub-processes created by the multiprocessing package.

the-knights-who-say-ni added the CLA not signed label Jan 21, 2018

bedevere-bot added the awaiting review label Jan 21, 2018

pppery reviewed Jan 21, 2018

View reviewed changes

bpo-32608: updated socketserver.rst based on pull-request comments su…

15ec16b

…ggesting ProcessingMixIn (and associated server classes) can still be used on non-POSIX systems.

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jan 23, 2018

deliberist added 2 commits February 10, 2018 17:21

bpo-32608: Adding documentation into Doc/whatsnew and Misc/NEWS.d/nex…

5444d6b

…t/Library.

bpo-32608: Fixed test_socketserver where it modified the environment …

f7de7b9

…after each test, which caused the tests to fail.

pitrou requested changes May 1, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels May 1, 2018

deliberist added 7 commits May 3, 2018 18:33

Merge remote-tracking branch 'upstream/master' into socketserver_and_…

535f13d

…multiprocessing # Conflicts: # Doc/whatsnew/3.7.rst

Merge remote-tracking branch 'upstream/master' into socketserver_and_…

f84538d

…multiprocessing

Merge remote-tracking branch 'upstream/master' into socketserver_and_…

5eee0ea

…multiprocessing

bpo-32608: Per PR-5258 comments: moved documentation to Python 3.8 fi…

cd209db

…les.

bpo-32608: Per PR-5258 comments: made the ProcessingMixIn ensure Proc…

28f397d

…ess.join() is called, rather than os.waitpid()

bpo-32608: Per PR-5258 comments, removed "multiprocessing.process._da…

22bdab7

…ngling" references; and fixed some test_socketserver tests.

bpo-32608: fixing errors with test_socketserver in a Windows environm…

9adc823

…ent.

deliberist added 3 commits May 7, 2018 12:08

bpo-32608: Fixing broken test_socketserver tests when run in Linux.

1f32fd7

bpo-32608: Fixing broken test_socketserver tests when run in Windows.

ef2fd3b

bpo-32608: Merge remote-tracking branch 'upstream/master' into socket…

7097ae6

…server_and_multiprocessing # Conflicts: # Doc/whatsnew/3.8.rst # Lib/socketserver.py

csabella requested a review from pitrou May 18, 2019 23:55

rbprogrammer added 3 commits March 16, 2021 19:48

Merge pull request #1 from python/master

61ef9f8

Updating fork with that of the upstream.

Merge branch 'socketserver_and_multiprocessing' into master

77d2cb2

Merge pull request #2 from rbprogrammer/master

18d14fd

Updating socketserver_and_multiprocessing with that of master.

rbprogrammer mannequin mentioned this pull request Apr 10, 2022

Incompatibilities with the socketserver and multiprocessing packages #76789

Open

vstinner added the topic-multiprocessing label Jun 6, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 14, 2022

github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 15, 2022

github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 27, 2023

		@@ -99,11 +99,12 @@ server classes.


		.. class:: ForkingMixIn
		ProcessingMixIn

Uh oh!

bpo-32608: Adding a server into socketserver that handles client connection in new "multiprocessing.Process" processes. #5258

Are you sure you want to change the base?

bpo-32608: Adding a server into socketserver that handles client connection in new "multiprocessing.Process" processes. #5258

Uh oh!

Conversation

deliberist commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jan 21, 2018

Uh oh!

deliberist commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deliberist Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deliberist commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deliberist commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 1, 2018

Uh oh!

deliberist commented May 6, 2018

Uh oh!

deliberist commented Jun 7, 2018

Uh oh!

vstinner commented Jun 8, 2018

Uh oh!

deliberist commented Jun 8, 2018

Uh oh!

github-actions bot commented Aug 14, 2022

Uh oh!

deliberist commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

Uh oh!

deliberist commented Jan 21, 2018 •

edited

Loading

deliberist Jan 23, 2018 •

edited

Loading

deliberist commented Feb 10, 2018 •

edited

Loading

deliberist commented Feb 10, 2018 •

edited

Loading

deliberist commented Aug 14, 2022 •

edited

Loading