Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race in test_docxmlrpc.py #71801

Closed
earlchew mannequin opened this issue Jul 25, 2016 · 12 comments
Closed

Race in test_docxmlrpc.py #71801

earlchew mannequin opened this issue Jul 25, 2016 · 12 comments
Labels
tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@earlchew
Copy link
Mannequin

earlchew mannequin commented Jul 25, 2016

BPO 27614
Nosy @bitdancer, @vadmium, @serhiy-storchaka
Files
  • test_docxmlrpc.py
  • setup-before-thread.patch
  • setup-before-thread.v2.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2016-08-20.09:24:51.681>
    created_at = <Date 2016-07-25.15:05:39.720>
    labels = ['type-bug', 'tests']
    title = 'Race in test_docxmlrpc.py'
    updated_at = <Date 2016-08-20.09:24:51.680>
    user = 'https://bugs.python.org/earlchew'

    bugs.python.org fields:

    activity = <Date 2016-08-20.09:24:51.680>
    actor = 'martin.panter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-08-20.09:24:51.681>
    closer = 'martin.panter'
    components = ['Tests']
    creation = <Date 2016-07-25.15:05:39.720>
    creator = 'earl.chew'
    dependencies = []
    files = ['43881', '44000', '44003']
    hgrepos = []
    issue_num = 27614
    keywords = ['patch']
    message_count = 12.0
    messages = ['271279', '271309', '271314', '271316', '271376', '271939', '271946', '271959', '272022', '272037', '272492', '273192']
    nosy_count = 5.0
    nosy_names = ['r.david.murray', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'earl.chew']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27614'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @earlchew
    Copy link
    Mannequin Author

    earlchew mannequin commented Jul 25, 2016

    The test test_docxmlrpc.py will sometimes hang because of a timing race.

    I've verified that this code is the same up to version 3.5 and master at https://github.com/python/cpython/blob/master/Lib/test/test_docxmlrpc.py

    A proposed patch is attached.

    @earlchew earlchew mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 25, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Jul 26, 2016

    I don’t understand why we have so many tests that assign the server port in the server thread, and then use some sort of synchronization to get it to the client thread. IMO it would be simpler in this case to do something like:

    def setUp(self):
        serv = DOCXMLRPCServer(...)
        self.addCleanup(serv.server_close)
        [_, PORT] = serv.server_address  # Eliminates “ready“ event
        # Other server setup here
        thread = threading.Thread(target=serv.serve_forever)
        thread.start()
        self.addCleanup(thread.join)  # Instead of self.evt
        self.addCleanup(serv.shutdown)
        self.client = httplib.HTTPConnection("localhost:%d" % PORT)
        self.addCleanup(self.client.close)

    @vadmium vadmium added tests Tests in the Lib/test dir and removed stdlib Python modules in the Lib dir labels Jul 26, 2016
    @bitdancer
    Copy link
    Member

    Because the real port number doesn't exist until the server thread starts running and binds the socket using port number 0.

    @vadmium
    Copy link
    Member

    vadmium commented Jul 26, 2016

    The whole point of my suggestion was to bind and set the server socket to listing mode before starting the other thread. The socketserver constructor should do this before returning:

    >>> s = DocXMLRPCServer(("localhost", 0))  # Calls bind() and listen()
    >>> s.server_address  # Non-zero port has been allocated
    ('127.0.0.1', 49806)
    >>> c = HTTPConnection(*s.server_address)
    >>> c.request("OPTIONS", "*")  # No connection refused error

    @bitdancer
    Copy link
    Member

    OK, that's a good point. So I don't know the answer to your question. In some cases it may be mostly that the tests are old and written when the tooling was not as good.

    @vadmium
    Copy link
    Member

    vadmium commented Aug 4, 2016

    Here is a patch with my idea of how it should work

    @vadmium
    Copy link
    Member

    vadmium commented Aug 4, 2016

    It seems the test infrastructure likes all references to thread objects to be deleted, even if they are no longer running.

    @serhiy-storchaka
    Copy link
    Member

    I have a doubt. Added a question on Rietveld.

    @vadmium
    Copy link
    Member

    vadmium commented Aug 5, 2016

    Earl: Can you give any more details on your original hang or race condition? Was it related to setting PORT, or shutting down the server, or something else? It is not clear from your patch. I have tried adding artificial sleep() calls at various points but that did not uncover anything.

    I’m sorry, but in my enthusiasm for rewriting the test I didn’t properly understand your original problem :)

    @earlchew
    Copy link
    Mannequin Author

    earlchew mannequin commented Aug 5, 2016

    In the original code, the key to the failure I observed is:

            # wait for port to be assigned
            n = 1000
            while n > 0 and PORT is None:
                time.sleep(0.001)
                n -= 1

    This gives a fixed deadline for the server thread to create the DocXMLRPCServer instance and provide the corresponding TCP port on which it is listening.

    If the server thread is late (or if an exception is thrown -- but this case might work out ok), the TCP port will not be available in the variable PORT. In this case, the client thread blunders on, and inadvertently fails because PORT == None.

    Upon failure, the test case tries to complete by tearing down the test:

         def tearDown(self):
             self.client.close()
     
             self.evt.wait()

    The test case waits for self.evt to indicate that the server has completed. However, the server thread is running this:

            while numrequests > 0:
                serv.handle_request()
                numrequests -= 1

    In other words, the test is deadlocked because the server is waiting to process requests (actually it's waiting for a single request) from the client, but the client has already given up and is waiting for the server thread to complete.

    @vadmium
    Copy link
    Member

    vadmium commented Aug 12, 2016

    Thanks for the explanation. It seems a bit strange that the server thread was running so slow while the main thread did one thousand polls over at least one second. Perhaps there is a blocking DNS call hidden somewhere in it somewhere? In any case, I am pretty confident my patch should help, so I will commit it unless there are any objections.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 20, 2016

    New changeset 397f05044172 by Martin Panter in branch '3.5':
    Issue bpo-27614: Avoid race in test_docxmlrpc server setup
    https://hg.python.org/cpython/rev/397f05044172

    New changeset 7136304ecf4c by Martin Panter in branch '2.7':
    Issue bpo-27614: Avoid race in test_docxmlrpc server setup
    https://hg.python.org/cpython/rev/7136304ecf4c

    New changeset 17d688dedfca by Martin Panter in branch 'default':
    Issue bpo-27614: Merge test_docxmlrpc from 3.5
    https://hg.python.org/cpython/rev/17d688dedfca

    @vadmium vadmium closed this as completed Aug 20, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants