1064960 - [mozprocess] Guard against the output thread trying to join with itself

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

In mozprocess, there is an output thread that consumes and processes output:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#795

When proc.wait() is called, we wait for this output thread to finish with join():
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#820

However, it's possible that someone puts a call to proc.wait() in an output, timeout, or finish handler. In this case wait is executed in the context of the output thread, and an exception is raised when the output thread tries to join with itself.

We should add a condition to this 'if' statement to make sure we are inside the context of the output thread:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#815

Comment 1

[Andrew Halberstadt [:ahal]]

Oops..

We should add a condition to this 'if' statement to make sure we are *not* inside the context of the output thread..

Comment 2

[Pankaj Malhotra (:bitgeeky)]

Looks Interesting, I'll like to work on this one.

Comment 3

[Pankaj Malhotra (:bitgeeky)]

Attachment: Bug-1064960.patch

I thought more on this, and what I think is that such a condition should ideally never arise because by design a thread must not join with itself.
So even if anyone tries to code in such a way that this condition arise it will definitely be wrong by design.

The original python code checks this condition like this:
https://hg.python.org/cpython/file/2.7/Lib/threading.py#l939

This is what we can also do here if we need to handle this case.

This is what I think, Please let me know if it sounds good to you also.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8515618 [details] [diff] [review]
Bug-1064960.patch

Review of attachment 8515618 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozprocess/mozprocess/processhandler.py
@@ +812,4 @@
>          - '0' if the process ended without failures
>  
>          """
> +        if self.outThread and self.outThread is not threading.current_thread():

I'm not that familiar with the outThread, but I wonder when the outThread can be the current thread. In line 795 we explicitly create a new thread, and the wait() method we only call from the main thread, right? So not sure under which conditions such a thing could happen.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8515618 [details] [diff] [review]
Bug-1064960.patch

Review of attachment 8515618 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, thanks!

Please upload a new patch that has the proper author information and a commit message formatted according to [1]. Let me know if you have any questions!

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> I'm not that familiar with the outThread, but I wonder when the outThread
> can be the current thread. In line 795 we explicitly create a new thread,
> and the wait() method we only call from the main thread, right? So not sure
> under which conditions such a thing could happen.

See comment 0, it can happen if someone calls proc.wait() from within an on_timeout or on_finish handler. It's not likely to happen, but we should still guard against it.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ah, I missed that. Thanks Andrew!

Comment 8

[Pankaj Malhotra (:bitgeeky)]

Attachment: Bug-1064960-v1.patch

I should have named the previous file as a diff rather than a patch :-) Here is the new one ! Thanks for the review :D

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8516600 [details] [diff] [review]
Bug-1064960-v1.patch

Review of attachment 8516600 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! I'll push this to try just in case.

Comment 10

[Andrew Halberstadt [:ahal]]

Try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7832df724df7

Comment 11

[Andrew Halberstadt [:ahal]]

Thanks for the contribution!

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/318f0e701234

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/318f0e701234

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/318f0e701234