Closed Bug 1410883 Opened 4 years ago Closed 4 years ago

[mozprocess] On some systems kill() is async and needs a following call to wait()

Categories

(Testing :: Mozbase, enhancement)

57 Branch
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1438009

People

(Reporter: whimboo, Unassigned)

References

Details

This has originally been seen on bug 1407203, and has been fixed for mochitest only via https://hg.mozilla.org/mozilla-central/rev/5fe4aa159e60.

But I think that we need a solution in mozprocess itself so that not each and every consumer of mozprocess has to do it itself.

With the fix in place we also want to revert the change from bug 1407203.
Ok, here some observations:

1) The ProcessHandler `kill()` method [1] calls `proc.kill` for the managed process, and then in line 768 it also calls `self.wait()`.

2) `self.wait()` [2] first waits for the reader thread to be gone. But then it also waits for the process to be gone. See line 853.

So I wonder why a separate `wait` call is needed here at all. The details on bug 1407203 are a bit spare so I cannot read the intention of doing that out of the comments. Maybe Ted and Daniel can clear this up.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#760 
[2] https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#828
Flags: needinfo?(ted)
Flags: needinfo?(dminor)
I missed the fact that proc.wait was called because I was focusing on the winprocess stuff above. The second call shouldn't be needed. If it is having an effect on Bug 1407203 then I guess it bandaids over a bug somewhere else.
Flags: needinfo?(dminor)
No longer blocks: 1401498
See Also: → 1415290
I don't have any additional info here, sorry.
Flags: needinfo?(ted)
I checked it all again, and meanwhile I think there is nothing missing in mozprocess in regards of a missing `wait()` call. Instead bug 1438009 should be the underlying problem here, which causes us to return too early from various methods like `kill()` because:

1) We don't take care of the returncode 259 which means STILL_ACTIVE and we would have to wait
2) A race condition where TerminateJobObject/TerminateProcess returns too early 

I will mark this bug as dupe of bug 1438009.
Assignee: hskupin → nobody
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1438009
You need to log in before you can comment on or make changes to this bug.