mozdevice can hang when running a process with lots of stdout

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: davehunt, Assigned: davehunt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
As we're using subprocess.Popen and specifying stdout=subprocess.PIPE we are hitting a rather nasty issue when there's over 65K of data pushed to stdout. I recently encountered this in b2gpopulate while pushing ~500 SMS attachments.

There's a good description of the issue here:
http://thraxil.org/users/anders/posts/2008/03/13/Subprocess-Hanging-PIPE-is-your-enemy/

From the subprocess documentations at: http://docs.python.org/2/library/subprocess.html#subprocess.Popen.communicate
"The data read is buffered in memory, so do not use this method if the data size is large or unlimited."

I've encountered this elsewhere, and in those cases I've added a log file written to disk. In this case I suspect we want to dismiss the stdout entirely, so perhaps read into memory or a temporary file and then remove?
I think jhammel also filed a bug about this for mozprocess, but I can't seem to find it.
(Assignee)

Comment 2

5 years ago
That would be bug 861951
(Assignee)

Comment 3

5 years ago
Created attachment 781647 [details] [diff] [review]
bug892882.patch
Attachment #781647 - Flags: feedback?(wlachance)
Comment on attachment 781647 [details] [diff] [review]
bug892882.patch

I like the idea of this, assuming it works!
Attachment #781647 - Flags: feedback?(wlachance) → feedback+
(Assignee)

Updated

5 years ago
Duplicate of this bug: 909265
(Assignee)

Comment 6

5 years ago
I've tested it successfully locally, but haven't had time to put together a mozdevice test for it yet.
I think we should try to land a variation of this patch, we don't have any unit tests for devicemanagerADB at this point so the testing situation isn't a big deal. My only comment is I think it would be better to use "with" to scope the temporary file rather than closing it manually.
(Assignee)

Comment 8

5 years ago
Created attachment 800411 [details] [diff] [review]
Use a temporary file for stdout. v2.0

This works well for my use case. There are other instances that we might want to change but I'm not confident doing so without tests or a better understanding of where/how they're used.
Assignee: nobody → dave.hunt
Attachment #781647 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #800411 - Flags: review?(wlachance)
Comment on attachment 800411 [details] [diff] [review]
Use a temporary file for stdout. v2.0

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

Looks good to me.
Attachment #800411 - Flags: review?(wlachance) → review+
(Assignee)

Comment 10

5 years ago
Landed as:
https://github.com/mozilla/mozbase/commit/4c02115648dd7f7f5e209f59d694815e41e8fe38
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
The Python documentation just seems wrong here. I would not call 65k "large".
You need to log in before you can comment on or make changes to this bug.