Closed Bug 1064960 Opened 5 years ago Closed 5 years ago

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

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: ahal, Assigned: bitgeeky, Mentored)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 1 obsolete file)

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
Oops..

We should add a condition to this 'if' statement to make sure we are *not* inside the context of the output thread..
Looks Interesting, I'll like to work on this one.
Assignee: nobody → mozpankaj1994
Attached patch Bug-1064960.patch (obsolete) — Splinter Review
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.
Attachment #8515618 - Flags: review?(hskupin)
Attachment #8515618 - Flags: review?(ahalberstadt)
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.
Attachment #8515618 - Flags: review?(hskupin)
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
Attachment #8515618 - Flags: review?(ahalberstadt) → review+
(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.
Ah, I missed that. Thanks Andrew!
I should have named the previous file as a diff rather than a patch :-) Here is the new one ! Thanks for the review :D
Attachment #8515618 - Attachment is obsolete: true
Attachment #8516600 - Flags: review?(ahalberstadt)
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.
Attachment #8516600 - Flags: review?(ahalberstadt) → review+
Flags: needinfo?(ahalberstadt)
Thanks for the contribution!
Flags: needinfo?(ahalberstadt)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/318f0e701234
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.