229824 2021-09-02 12:42:43 -0700 [webkitpy] WrappedPopen breaks process returncode 2021-09-02 14:10:31 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=201826 https://bugs.webkit.org/show_bug.cgi?id=229758 InRadar P2 Normal --- 1 clopez jbedard ews-watchlist glenn jbedard webkit-bug-importer oldest_to_newest 1789846 0 clopez 2021-09-02 12:42:43 -0700 The class WrappedPopen() was added in r250375 to wrap the subprocess.Popen() for making the python2 subprocess.Popen() objects more similar to the ones from python3. But the way this class wraps the objects makes breaks the process returncode if the process doesn't return immediately. This is because WrappedPopen() defines a new class the sets functions with the same name than subprocess.Popen() setting the value returned to the one that returns subprocess.Popen(). But in the case of subprocess.Popen().returncode that is not a function, but a string that will change of value when the process ends. If we evaluate this function before we call subprocess.Popen().communicate() or subprocess.Popen().wait() then the value of returncode will be None This can be checked with this following test program: http://sprunge.us/aoxZ0j If you run it with python2 you get "The call return code is None" instead of getting "The call return code is 2" which is what it should be (what happens when you run it with python3 which don't triggers the call to use WrappedPopen()) I have observed this when working on bug 229758 .. with python2 the call from host.executive.popen() always returned a returncode of None This is not an issue for subprocess.Popen().stdout and subprocess.Popen().stderr because if you look at what prints the debug function on that example for this cases it sets the value to a descriptor rather than reading from the descriptor. So when the call to read from the descriptor is done it will work. Example, you see this: setting attribute .. stdout to <open file '<fdopen>', mode 'rb' at 0x7fe12cc1d540> setting attribute .. returncode to None 1789850 1 437186 jbedard 2021-09-02 12:55:48 -0700 Created attachment 437186 Patch 1789851 2 jbedard 2021-09-02 12:56:37 -0700 (In reply to Jonathan Bedard from comment #1) > Created attachment 437186 [details] > Patch I suspect this will fix the problem, although I'm a bit unclear which script can cause this. 1789864 3 clopez 2021-09-02 13:25:20 -0700 This fixes it indeed, thanks. I was trying here to instead of wrapping the object to add the __exit__ and __enter__ methods to the Popen class directly, but for some unknown reason then when you try to use the usual "with popen_object:" it doesn't like this methods added via setattr() and complains about missing __exit__ 1789885 4 ews-feeder 2021-09-02 14:09:22 -0700 Committed r281952 (241259@main): <https://commits.webkit.org/241259@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437186. 1789888 5 webkit-bug-importer 2021-09-02 14:10:31 -0700 <rdar://problem/82692759> 437186 2021-09-02 12:55:48 -0700 2021-09-02 14:09:22 -0700 Patch bug-229824-20210902125548.patch text/plain 1711 jbedard U3VidmVyc2lvbiBSZXZpc2lvbjogMjgxOTQ3CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggY2Q5OGJjYmM5NTVmZjhhNGUyM2U1MTlkNTk1Y2M5NTc5 MWI5MDA2YS4uOWUyN2U3ODZhNzA2YmRkZmM3NWNlNTRlNDNlNTBjZWExYmU3MmE1MyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0 IEBACisyMDIxLTA5LTAyICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNvbT4KKwor ICAgICAgICBbd2Via2l0cHldIFdyYXBwZWRQb3BlbiBicmVha3MgcHJvY2VzcyByZXR1cm5jb2Rl CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMjk4MjQK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIFNjcmlw dHMvd2Via2l0cHkvY29tbW9uL3N5c3RlbS9leGVjdXRpdmUucHk6CisgICAgICAgIChXcmFwcGVk UG9wZW4uX19pbml0X18pOiBEZWZpbmUgdW5kZXJseWluZyBvYmplY3QuCisgICAgICAgIChXcmFw cGVkUG9wZW4ucmV0dXJuY29kZSk6IFJldHVybiB1bmRlcmx5aW5nIG9iamVjdCdzIHJldHVybmNv ZGUuCisKIDIwMjEtMDktMDIgIENhcmxvcyBBbGJlcnRvIExvcGV6IFBlcmV6ICA8Y2xvcGV6QGln YWxpYS5jb20+CiAKICAgICAgICAgW0dUS11bV1BFXSBQb3J0IEFQSSB0ZXN0IHJ1bm5lciB0byBw eXRob24zICh2MikKZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5 c3RlbS9leGVjdXRpdmUucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0v ZXhlY3V0aXZlLnB5CmluZGV4IDU1NTZjNDQ1MDRkNzhiM2Y4MmNjNTJkYzM2MDUxOTIyMmNkMWU1 NGMuLjQ3YzhmMTE0MTg5ODYzODQ3YWY4MWE1MWRkM2I5MjE2YThiOTEzNGQgMTAwNjQ0Ci0tLSBh L1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3RlbS9leGVjdXRpdmUucHkKKysrIGIv VG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vc3lzdGVtL2V4ZWN1dGl2ZS5weQpAQCAtODMs MTAgKzgzLDE1IEBAIGNsYXNzIFNjcmlwdEVycm9yKEV4Y2VwdGlvbik6CiAKIGNsYXNzIFdyYXBw ZWRQb3BlbihvYmplY3QpOgogICAgIGRlZiBfX2luaXRfXyhzZWxmLCBwb3Blbik6Ci0gICAgICAg IGZvciBhdHRyaWJ1dGUgaW4gZGlyKHBvcGVuKToKLSAgICAgICAgICAgIGlmIGF0dHJpYnV0ZS5z dGFydHN3aXRoKCdfXycpOgorICAgICAgICBzZWxmLl9wb3BlbiA9IHBvcGVuCisgICAgICAgIGZv ciBhdHRyaWJ1dGUgaW4gZGlyKHNlbGYuX3BvcGVuKToKKyAgICAgICAgICAgIGlmIGF0dHJpYnV0 ZS5zdGFydHN3aXRoKCdfXycpIG9yIGF0dHJpYnV0ZSA9PSAncmV0dXJuY29kZSc6CiAgICAgICAg ICAgICAgICAgY29udGludWUKLSAgICAgICAgICAgIHNldGF0dHIoc2VsZiwgYXR0cmlidXRlLCBn ZXRhdHRyKHBvcGVuLCBhdHRyaWJ1dGUpKQorICAgICAgICAgICAgc2V0YXR0cihzZWxmLCBhdHRy aWJ1dGUsIGdldGF0dHIoc2VsZi5fcG9wZW4sIGF0dHJpYnV0ZSkpCisKKyAgICBAcHJvcGVydHkK KyAgICBkZWYgcmV0dXJuY29kZShzZWxmKToKKyAgICAgICAgcmV0dXJuIHNlbGYuX3BvcGVuLnJl dHVybmNvZGUKIAogICAgIGRlZiBfX2VudGVyX18oc2VsZik6CiAgICAgICAgIHJldHVybiBzZWxm Cg==