217632 2020-10-12 14:06:26 -0700 [webkitcorepy] Attempt to terminate stuck processes before killing them 2020-10-12 15:55:00 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jbedard jbedard aakash_jain dewei_zhu slewis webkit-bug-importer oldest_to_newest 1697013 0 jbedard 2020-10-12 14:06:26 -0700 I encountered a case today where simply killing a stuck process was not sufficient, the process needed to be terminated. In order for us to trust webkitcorepy.run, we should terminate processes when they time out. 1697015 1 webkit-bug-importer 2020-10-12 14:06:43 -0700 <rdar://problem/70222803> 1697017 2 411153 jbedard 2020-10-12 14:09:06 -0700 Created attachment 411153 Patch 1697028 3 411153 dewei_zhu 2020-10-12 14:24:51 -0700 Comment on attachment 411153 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411153&action=review > Tools/ChangeLog:11 > + (run): Terminate stuck processes instead of killing them since termination is guaranteed to work. Are we sure about this? Because `terminate` only sends SIGTERM, but `kill` sends SIGKILL, which is more powerful than the former. Per https://docs.python.org/3/library/subprocess.html#subprocess.Popen.terminate & https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html 1697038 4 jbedard 2020-10-12 14:43:53 -0700 (In reply to dewei_zhu from comment #3) > Comment on attachment 411153 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=411153&action=review > > > Tools/ChangeLog:11 > > + (run): Terminate stuck processes instead of killing them since termination is guaranteed to work. > > Are we sure about this? Because `terminate` only sends SIGTERM, but `kill` > sends SIGKILL, which is more powerful than the former. > Per > https://docs.python.org/3/library/subprocess.html#subprocess.Popen.terminate > & https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html Oops! You're right, it's the communicate that's our issue 1697050 5 411163 jbedard 2020-10-12 14:54:18 -0700 Created attachment 411163 Patch 1697057 6 jbedard 2020-10-12 14:57:38 -0700 To be a bit more specific, this was found with `git svn` being run from a repository that doesn't support it. 1697070 7 411163 dewei_zhu 2020-10-12 15:17:12 -0700 Comment on attachment 411163 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411163&action=review > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/subprocess_utils.py:137 > process.wait() Not sure if this is related to the initial problem this patch intends to address. Would this cause deadlock when 1. timeout is specified & 2. huge amount of output is produced to subprocess.PIPE 3. program hits the timeout Per the warning from https://docs.python.org/2.7/library/subprocess.html#subprocess.Popen.wait 1697076 8 411163 jbedard 2020-10-12 15:26:35 -0700 Comment on attachment 411163 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411163&action=review >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/subprocess_utils.py:137 >> process.wait() > > Not sure if this is related to the initial problem this patch intends to address. > Would this cause deadlock when > 1. timeout is specified & > 2. huge amount of output is produced to subprocess.PIPE > 3. program hits the timeout > Per the warning from https://docs.python.org/2.7/library/subprocess.html#subprocess.Popen.wait We shouldn't have to worry about that case because we've already killed the process and attempted to communicate....actually, I suspect that we run a risk of hanging a program in that case on line 129, but it shouldn't be a deadlock, it should just be until we consume the buffer. That being said, the existing code had the same problem, so I think we should defer such a fix until we have evidence we actually need it. 1697097 9 ews-feeder 2020-10-12 15:54:59 -0700 Committed r268373: <https://trac.webkit.org/changeset/268373> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411163. 411153 2020-10-12 14:09:06 -0700 2020-10-12 14:54:17 -0700 Patch bug-217632-20201012140905.patch text/plain 2368 jbedard SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDI2ODM2NykKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE1IEBACisyMDIwLTEwLTEyICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNv bT4KKworICAgICAgICBbd2Via2l0Y29yZXB5XSBUZXJtaW5hdGUgc3R1Y2sgcHJvY2Vzc2VzIGlu c3RlYWQgb2Yga2lsbGluZyB0aGVtCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0yMTc2MzIKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzcwMjIyODAzPgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogU2NyaXB0 cy9saWJyYXJpZXMvd2Via2l0Y29yZXB5L3dlYmtpdGNvcmVweS9fX2luaXRfXy5weTogQnVtcCB2 ZXJzaW9uLgorICAgICAgICAqIFNjcmlwdHMvbGlicmFyaWVzL3dlYmtpdGNvcmVweS93ZWJraXRj b3JlcHkvc3VicHJvY2Vzc191dGlscy5weToKKyAgICAgICAgKHJ1bik6IFRlcm1pbmF0ZSBzdHVj ayBwcm9jZXNzZXMgaW5zdGVhZCBvZiBraWxsaW5nIHRoZW0gc2luY2UgdGVybWluYXRpb24gaXMg Z3VhcmFudGVlZCB0byB3b3JrLgorCiAyMDIwLTEwLTEyICBMdW1pbmcgWWluICA8bHVtaW5nX3lp bkBhcHBsZS5jb20+CiAKICAgICAgICAgW21hY09TXSBXb3JrYXJvdW5kIGZvciBNQUNfT1NfWF9W RVJTSU9OX01BSk9SIGluY29ycmVjdGx5IGluY2x1ZGluZyBtaW5vciB2ZXJzaW9uIHdoZW4gYnVp bGRpbmcgCkluZGV4OiBUb29scy9TY3JpcHRzL2xpYnJhcmllcy93ZWJraXRjb3JlcHkvd2Via2l0 Y29yZXB5L19faW5pdF9fLnB5Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xzL1NjcmlwdHMvbGlicmFyaWVz L3dlYmtpdGNvcmVweS93ZWJraXRjb3JlcHkvX19pbml0X18ucHkJKHJldmlzaW9uIDI2ODM2NykK KysrIFRvb2xzL1NjcmlwdHMvbGlicmFyaWVzL3dlYmtpdGNvcmVweS93ZWJraXRjb3JlcHkvX19p bml0X18ucHkJKHdvcmtpbmcgY29weSkKQEAgLTM1LDcgKzM1LDcgQEAgZnJvbSB3ZWJraXRjb3Jl cHkudGltZW91dCBpbXBvcnQgVGltZW91dAogZnJvbSB3ZWJraXRjb3JlcHkuc3VicHJvY2Vzc191 dGlscyBpbXBvcnQgVGltZW91dEV4cGlyZWQsIENvbXBsZXRlZFByb2Nlc3MsIHJ1bgogZnJvbSB3 ZWJraXRjb3JlcHkub3V0cHV0X2NhcHR1cmUgaW1wb3J0IExvZ2dlckNhcHR1cmUsIE91dHB1dENh cHR1cmUsIE91dHB1dER1cGxpY2F0ZQogCi12ZXJzaW9uID0gVmVyc2lvbigwLCA0LCAxMykKK3Zl cnNpb24gPSBWZXJzaW9uKDAsIDQsIDE0KQogCiBmcm9tIHdlYmtpdGNvcmVweS5hdXRvaW5zdGFs bCBpbXBvcnQgUGFja2FnZSwgQXV0b0luc3RhbGwKIGlmIHN5cy52ZXJzaW9uX2luZm8gPiAoMywg MCk6CkluZGV4OiBUb29scy9TY3JpcHRzL2xpYnJhcmllcy93ZWJraXRjb3JlcHkvd2Via2l0Y29y ZXB5L3N1YnByb2Nlc3NfdXRpbHMucHkKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvU2NyaXB0cy9saWJy YXJpZXMvd2Via2l0Y29yZXB5L3dlYmtpdGNvcmVweS9zdWJwcm9jZXNzX3V0aWxzLnB5CShyZXZp c2lvbiAyNjgzNjcpCisrKyBUb29scy9TY3JpcHRzL2xpYnJhcmllcy93ZWJraXRjb3JlcHkvd2Vi a2l0Y29yZXB5L3N1YnByb2Nlc3NfdXRpbHMucHkJKHdvcmtpbmcgY29weSkKQEAgLTExOCwxMSAr MTE4LDExIEBAIGVsc2U6CiAgICAgICAgICAgICAgICAgc3Rkb3V0LCBzdGRlcnIgPSBwcm9jZXNz LmNvbW11bmljYXRlKGlucHV0KQogCiAgICAgICAgIGV4Y2VwdCBUaW1lb3V0LkV4Y2VwdGlvbjoK LSAgICAgICAgICAgIHByb2Nlc3Mua2lsbCgpCisgICAgICAgICAgICBwcm9jZXNzLnRlcm1pbmF0 ZSgpCiAgICAgICAgICAgICBzdGRvdXQsIHN0ZGVyciA9IHByb2Nlc3MuY29tbXVuaWNhdGUoKQog ICAgICAgICAgICAgcmFpc2UgVGltZW91dEV4cGlyZWQocG9wZW5hcmdzWzBdLCB0aW1lb3V0LCBv dXRwdXQ9ZGVjb2RlKHN0ZG91dCksIHN0ZGVycj1kZWNvZGUoc3RkZXJyKSkKICAgICAgICAgZXhj ZXB0IEJhc2VFeGNlcHRpb246Ci0gICAgICAgICAgICBwcm9jZXNzLmtpbGwoKQorICAgICAgICAg ICAgcHJvY2Vzcy50ZXJtaW5hdGUoKQogICAgICAgICAgICAgcmFpc2UKICAgICAgICAgZmluYWxs eToKICAgICAgICAgICAgIHByb2Nlc3Mud2FpdCgpCg== 411163 2020-10-12 14:54:18 -0700 2020-10-12 15:54:59 -0700 Patch bug-217632-20201012145418.patch text/plain 2686 jbedard SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDI2ODM2OSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE1IEBACisyMDIwLTEwLTEyICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNv bT4KKworICAgICAgICBbd2Via2l0Y29yZXB5XSBBdHRlbXB0IHRvIHRlcm1pbmF0ZSBzdHVjayBw cm9jZXNzZXMgYmVmb3JlIGtpbGxpbmcgdGhlbQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE3NjMyCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS83MDIy MjgwMz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAq IFNjcmlwdHMvbGlicmFyaWVzL3dlYmtpdGNvcmVweS93ZWJraXRjb3JlcHkvX19pbml0X18ucHk6 IEJ1bXAgdmVyc2lvbi4KKyAgICAgICAgKiBTY3JpcHRzL2xpYnJhcmllcy93ZWJraXRjb3JlcHkv d2Via2l0Y29yZXB5L3N1YnByb2Nlc3NfdXRpbHMucHk6CisgICAgICAgIChydW4pOiBLaWxsZWQg cHJvY2Vzc2VzIHdpbGwgc29tZXRpbWVzIG5vdCBoYXZlIHN0ZG91dCBvciBzdGRlcnIsIHdoaWxl IHRlcm1pbmF0ZWQgcHJvY2Vzc2VzIG9mdGVuIHdpbGwuCisKIDIwMjAtMTAtMTIgIEx1bWluZyBZ aW4gIDxsdW1pbmdfeWluQGFwcGxlLmNvbT4KIAogICAgICAgICBbbWFjT1NdIFdvcmthcm91bmQg Zm9yIE1BQ19PU19YX1ZFUlNJT05fTUFKT1IgaW5jb3JyZWN0bHkgaW5jbHVkaW5nIG1pbm9yIHZl cnNpb24gd2hlbiBidWlsZGluZyAKSW5kZXg6IFRvb2xzL1NjcmlwdHMvbGlicmFyaWVzL3dlYmtp dGNvcmVweS93ZWJraXRjb3JlcHkvX19pbml0X18ucHkKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvU2Ny aXB0cy9saWJyYXJpZXMvd2Via2l0Y29yZXB5L3dlYmtpdGNvcmVweS9fX2luaXRfXy5weQkocmV2 aXNpb24gMjY4MzY5KQorKysgVG9vbHMvU2NyaXB0cy9saWJyYXJpZXMvd2Via2l0Y29yZXB5L3dl YmtpdGNvcmVweS9fX2luaXRfXy5weQkod29ya2luZyBjb3B5KQpAQCAtMzUsNyArMzUsNyBAQCBm cm9tIHdlYmtpdGNvcmVweS50aW1lb3V0IGltcG9ydCBUaW1lb3V0CiBmcm9tIHdlYmtpdGNvcmVw eS5zdWJwcm9jZXNzX3V0aWxzIGltcG9ydCBUaW1lb3V0RXhwaXJlZCwgQ29tcGxldGVkUHJvY2Vz cywgcnVuCiBmcm9tIHdlYmtpdGNvcmVweS5vdXRwdXRfY2FwdHVyZSBpbXBvcnQgTG9nZ2VyQ2Fw dHVyZSwgT3V0cHV0Q2FwdHVyZSwgT3V0cHV0RHVwbGljYXRlCiAKLXZlcnNpb24gPSBWZXJzaW9u KDAsIDQsIDEzKQordmVyc2lvbiA9IFZlcnNpb24oMCwgNCwgMTQpCiAKIGZyb20gd2Via2l0Y29y ZXB5LmF1dG9pbnN0YWxsIGltcG9ydCBQYWNrYWdlLCBBdXRvSW5zdGFsbAogaWYgc3lzLnZlcnNp b25faW5mbyA+ICgzLCAwKToKSW5kZXg6IFRvb2xzL1NjcmlwdHMvbGlicmFyaWVzL3dlYmtpdGNv cmVweS93ZWJraXRjb3JlcHkvc3VicHJvY2Vzc191dGlscy5weQo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29s cy9TY3JpcHRzL2xpYnJhcmllcy93ZWJraXRjb3JlcHkvd2Via2l0Y29yZXB5L3N1YnByb2Nlc3Nf dXRpbHMucHkJKHJldmlzaW9uIDI2ODM2OSkKKysrIFRvb2xzL1NjcmlwdHMvbGlicmFyaWVzL3dl YmtpdGNvcmVweS93ZWJraXRjb3JlcHkvc3VicHJvY2Vzc191dGlscy5weQkod29ya2luZyBjb3B5 KQpAQCAtMTE4LDEyICsxMTgsMjEgQEAgZWxzZToKICAgICAgICAgICAgICAgICBzdGRvdXQsIHN0 ZGVyciA9IHByb2Nlc3MuY29tbXVuaWNhdGUoaW5wdXQpCiAKICAgICAgICAgZXhjZXB0IFRpbWVv dXQuRXhjZXB0aW9uOgotICAgICAgICAgICAgcHJvY2Vzcy5raWxsKCkKLSAgICAgICAgICAgIHN0 ZG91dCwgc3RkZXJyID0gcHJvY2Vzcy5jb21tdW5pY2F0ZSgpCisgICAgICAgICAgICBwcm9jZXNz LnRlcm1pbmF0ZSgpCisgICAgICAgICAgICBkZWFkbGluZSA9IHRpbWUudGltZSgpICsgLjUKKyAg ICAgICAgICAgIHdoaWxlIHByb2Nlc3MucG9sbCgpIGlzIE5vbmUgYW5kIHRpbWUudGltZSgpIDwg ZGVhZGxpbmU6CisgICAgICAgICAgICAgICAgdGltZS5zbGVlcCguMSkKKyAgICAgICAgICAgIGlm IHByb2Nlc3MucG9sbCgpIGlzIE5vbmU6CisgICAgICAgICAgICAgICAgcHJvY2Vzcy5raWxsKCkK KyAgICAgICAgICAgICAgICBzdGRvdXQsIHN0ZGVyciA9ICcnLCAnJworICAgICAgICAgICAgZWxz ZToKKyAgICAgICAgICAgICAgICBzdGRvdXQsIHN0ZGVyciA9IHByb2Nlc3MuY29tbXVuaWNhdGUo KQogICAgICAgICAgICAgcmFpc2UgVGltZW91dEV4cGlyZWQocG9wZW5hcmdzWzBdLCB0aW1lb3V0 LCBvdXRwdXQ9ZGVjb2RlKHN0ZG91dCksIHN0ZGVycj1kZWNvZGUoc3RkZXJyKSkKKwogICAgICAg ICBleGNlcHQgQmFzZUV4Y2VwdGlvbjoKICAgICAgICAgICAgIHByb2Nlc3Mua2lsbCgpCiAgICAg ICAgICAgICByYWlzZQorCiAgICAgICAgIGZpbmFsbHk6CiAgICAgICAgICAgICBwcm9jZXNzLndh aXQoKQogCg==