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.
That would be bug 861951
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+
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.
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.
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+
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.