48321 2010-10-26 06:55:22 -0700 [NRWT] Fix http lock on Windows platform 2010-10-27 10:51:41 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 48053 1 rgabor webkit-unassigned abecsi commit-queue dpranke eric ojan ossy tony oldest_to_newest 299413 0 rgabor 2010-10-26 06:55:22 -0700 Http lock doesn't work on Windows platform. 299423 1 71875 rgabor 2010-10-26 07:11:26 -0700 Created attachment 71875 proposed patch 299713 2 71875 dpranke 2010-10-26 12:34:35 -0700 Comment on attachment 71875 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=71875&action=review LGTM with the comment fixed as per below. > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:75 > + FIXME: On Windows it's always return True.""" Nit: I'd rewrite this as "FIXME: os.kill() doesn't work on Windows for checking if a pid is alive, so always return True" 299719 3 71875 tony 2010-10-26 12:38:35 -0700 Comment on attachment 71875 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=71875&action=review > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:78 > + FIXME: On Windows it's always return True.""" > + if sys.platform in ('darwin', 'linux2'): > + try: > + os.kill(current_pid, 0) In webkitpy/common/system/executive.py, there's a kill_process function that should work on windows. Can we use that instead? > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:108 > + if sys.platform in ('darwin', 'linux2'): > + guard_lock_flags |= os.O_NONBLOCK Should we add 'cygwin' to the platform tuple? It looks like cygwin python has os.O_NONBLOCK, although I'm not sure if it does anything. 299724 4 dpranke 2010-10-26 12:43:47 -0700 (In reply to comment #3) > (From update of attachment 71875 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71875&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:78 > > + FIXME: On Windows it's always return True.""" > > + if sys.platform in ('darwin', 'linux2'): > > + try: > > + os.kill(current_pid, 0) > > In webkitpy/common/system/executive.py, there's a kill_process function that should work on windows. Can we use that instead? > No. That routine actually kills the process. This routine is just probing the process to see if it's alive. Frankly, I'm not a fan of using kill -0 to do that, but there doesn't seem to be an alternative that doesn't require shelling out to ps or invoking WMI on windows. > > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:108 > > + if sys.platform in ('darwin', 'linux2'): > > + guard_lock_flags |= os.O_NONBLOCK > > Should we add 'cygwin' to the platform tuple? It looks like cygwin python has os.O_NONBLOCK, although I'm not sure if it does anything. I doubt it does anything. 299744 5 eric 2010-10-26 13:00:45 -0700 We started the process. So we should have a Popen object no? If so, we can just use poll() to check if a process is alive. 299899 6 dpranke 2010-10-26 17:08:05 -0700 (In reply to comment #5) > We started the process. So we should have a Popen object no? If so, we can just use poll() to check if a process is alive. Not if you're running multiple NRWTs and one of the other ones is the one holding the lock ... 300112 7 72003 rgabor 2010-10-27 04:20:19 -0700 Created attachment 72003 proposed_patch_v2 As Dirk said I don't want to kill the process, just check it. I think that's the easiest way to do that. Furthermore I've checked the os.O_NONBLOCK and it's no need because when it can't open the guard lock file it just throw an OSError exception, so it will be a spinlock without the os.O_NONBLOCK. 300360 8 72003 commit-queue 2010-10-27 10:51:35 -0700 Comment on attachment 72003 proposed_patch_v2 Clearing flags on attachment: 72003 Committed r70672: <http://trac.webkit.org/changeset/70672> 300361 9 commit-queue 2010-10-27 10:51:41 -0700 All reviewed patches have been landed. Closing bug. 71875 2010-10-26 07:11:26 -0700 2010-10-27 04:20:19 -0700 proposed patch http_lock_fix.patch text/plain 2824 rgabor SW5kZXg6IFdlYktpdFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXRUb29scy9D aGFuZ2VMb2cJKHJldmlzaW9uIDcwNTE4KQorKysgV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTIgQEAKKzIwMTAtMTAtMjYgIEdhYm9yIFJhcGNzYW55aSAg PHJnYWJvckBpbmYudS1zemVnZWQuaHU+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgW05SV1RdIEZpeCBodHRwIGxvY2sgb24gV2luZG93cyBwbGF0Zm9y bQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDgzMjEK KworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvaHR0cF9sb2Nr LnB5OgorCiAyMDEwLTEwLTI1ICBFcmljIFNlaWRlbCAgPGVyaWNAd2Via2l0Lm9yZz4KIAogICAg ICAgICBSZXZpZXdlZCBieSBBZGFtIEJhcnRoLgpJbmRleDogV2ViS2l0VG9vbHMvU2NyaXB0cy93 ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9odHRwX2xvY2sucHkKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2Vi S2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9odHRwX2xvY2sucHkJ KHJldmlzaW9uIDcwNTE3KQorKysgV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRf dGVzdHMvcG9ydC9odHRwX2xvY2sucHkJKHdvcmtpbmcgY29weSkKQEAgLTcxLDEzICs3MSwxNCBA QCBjbGFzcyBIdHRwTG9jayhvYmplY3QpOgogICAgICAgICByZXR1cm4gc2VsZi5fZXh0cmFjdF9s b2NrX251bWJlcihsb2NrX2xpc3RbLTFdKSArIDEKIAogICAgIGRlZiBfY2hlY2tfcGlkKHNlbGYs IGN1cnJlbnRfcGlkKToKLSAgICAgICAgIiIiUmV0dXJuIFRydWUgaWYgcGlkIGlzIGFsaXZlLCBv dGhlcndpc2UgcmV0dXJuIEZhbHNlLiIiIgotICAgICAgICB0cnk6Ci0gICAgICAgICAgICBvcy5r aWxsKGN1cnJlbnRfcGlkLCAwKQotICAgICAgICBleGNlcHQgT1NFcnJvcjoKLSAgICAgICAgICAg IHJldHVybiBGYWxzZQotICAgICAgICBlbHNlOgotICAgICAgICAgICAgcmV0dXJuIFRydWUKKyAg ICAgICAgIiIiUmV0dXJuIFRydWUgaWYgcGlkIGlzIGFsaXZlLCBvdGhlcndpc2UgcmV0dXJuIEZh bHNlLgorICAgICAgICBGSVhNRTogT24gV2luZG93cyBpdCdzIGFsd2F5cyByZXR1cm4gVHJ1ZS4i IiIKKyAgICAgICAgaWYgc3lzLnBsYXRmb3JtIGluICgnZGFyd2luJywgJ2xpbnV4MicpOgorICAg ICAgICAgICAgdHJ5OgorICAgICAgICAgICAgICAgIG9zLmtpbGwoY3VycmVudF9waWQsIDApCisg ICAgICAgICAgICBleGNlcHQgT1NFcnJvcjoKKyAgICAgICAgICAgICAgICByZXR1cm4gRmFsc2UK KyAgICAgICAgcmV0dXJuIFRydWUKIAogICAgIGRlZiBfY3VyZW50X2xvY2tfcGlkKHNlbGYpOgog ICAgICAgICAiIiJSZXR1cm4gd2l0aCB0aGUgY3VycmVudCBsb2NrIHBpZC4gSWYgdGhlIGxvY2sg aXMgbm90IHZhbGlkCkBAIC04OSw5ICs5MCw3IEBAIGNsYXNzIEh0dHBMb2NrKG9iamVjdCk6CiAg ICAgICAgICAgICBjdXJyZW50X2xvY2tfZmlsZSA9IG9wZW4obG9ja19saXN0WzBdLCAncicpCiAg ICAgICAgICAgICBjdXJyZW50X3BpZCA9IGN1cnJlbnRfbG9ja19maWxlLnJlYWRsaW5lKCkKICAg ICAgICAgICAgIGN1cnJlbnRfbG9ja19maWxlLmNsb3NlKCkKLSAgICAgICAgICAgIGlmIG5vdCAo Y3VycmVudF9waWQgYW5kCi0gICAgICAgICAgICAgIHN5cy5wbGF0Zm9ybSBpbiAoJ2Rhcndpbics ICdsaW51eDInKSBhbmQKLSAgICAgICAgICAgICAgc2VsZi5fY2hlY2tfcGlkKGludChjdXJyZW50 X3BpZCkpKToKKyAgICAgICAgICAgIGlmIG5vdCAoY3VycmVudF9waWQgYW5kIHNlbGYuX2NoZWNr X3BpZChpbnQoY3VycmVudF9waWQpKSk6CiAgICAgICAgICAgICAgICAgb3MudW5saW5rKGxvY2tf bGlzdFswXSkKICAgICAgICAgICAgICAgICByZXR1cm4KICAgICAgICAgZXhjZXB0IElPRXJyb3Is IE9TRXJyb3I6CkBAIC0xMDQsOCArMTAzLDEwIEBAIGNsYXNzIEh0dHBMb2NrKG9iamVjdCk6CiAg ICAgICAgIG51bWJlcnMgYXJlIHNlcXVlbnRpYWwuIiIiCiAgICAgICAgIHdoaWxlKFRydWUpOgog ICAgICAgICAgICAgdHJ5OgotICAgICAgICAgICAgICAgIHNlcXVlbnRpYWxfZ3VhcmRfbG9jayA9 IG9zLm9wZW4oc2VsZi5fZ3VhcmRfbG9ja19maWxlLAotICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgb3MuT19DUkVBVCB8IG9zLk9fTk9OQkxPQ0sgfCBvcy5P X0VYQ0wpCisgICAgICAgICAgICAgICAgZ3VhcmRfbG9ja19mbGFncyA9IG9zLk9fQ1JFQVQgfCBv cy5PX0VYQ0wKKyAgICAgICAgICAgICAgICBpZiBzeXMucGxhdGZvcm0gaW4gKCdkYXJ3aW4nLCAn bGludXgyJyk6CisgICAgICAgICAgICAgICAgICAgIGd1YXJkX2xvY2tfZmxhZ3MgfD0gb3MuT19O T05CTE9DSworICAgICAgICAgICAgICAgIHNlcXVlbnRpYWxfZ3VhcmRfbG9jayA9IG9zLm9wZW4o c2VsZi5fZ3VhcmRfbG9ja19maWxlLCBndWFyZF9sb2NrX2ZsYWdzKQogCiAgICAgICAgICAgICAg ICAgc2VsZi5fcHJvY2Vzc19sb2NrX2ZpbGVfbmFtZSA9IChzZWxmLl9sb2NrX2ZpbGVfcGF0aF9w cmVmaXggKwogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg c3RyKHNlbGYuX25leHRfbG9ja19udW1iZXIoKSkpCg== 72003 2010-10-27 04:20:19 -0700 2010-10-27 10:51:35 -0700 proposed_patch_v2 http_lock_fix2.patch text/plain 2793 rgabor SW5kZXg6IFdlYktpdFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXRUb29scy9D aGFuZ2VMb2cJKHJldmlzaW9uIDcwNjI3KQorKysgV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTIgQEAKKzIwMTAtMTAtMjcgIEdhYm9yIFJhcGNzYW55aSAg PHJnYWJvckBpbmYudS1zemVnZWQuaHU+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgW05SV1RdIEZpeCBodHRwIGxvY2sgb24gV2luZG93cyBwbGF0Zm9y bQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDgzMjEK KworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvaHR0cF9sb2Nr LnB5OgorCiAyMDEwLTEwLTI2ICBBbnRvbmlvIEdvbWVzICA8YWdvbWVzQHJpbS5jb20+CiAKICAg ICAgICAgUmV2aWV3ZWQgYnkgTWFydGluIFJvYmluc29uLgpJbmRleDogV2ViS2l0VG9vbHMvU2Ny aXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9odHRwX2xvY2sucHkKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9odHRwX2xv Y2sucHkJKHJldmlzaW9uIDcwNjI3KQorKysgV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9s YXlvdXRfdGVzdHMvcG9ydC9odHRwX2xvY2sucHkJKHdvcmtpbmcgY29weSkKQEAgLTcxLDEzICs3 MSwxNSBAQCBjbGFzcyBIdHRwTG9jayhvYmplY3QpOgogICAgICAgICByZXR1cm4gc2VsZi5fZXh0 cmFjdF9sb2NrX251bWJlcihsb2NrX2xpc3RbLTFdKSArIDEKIAogICAgIGRlZiBfY2hlY2tfcGlk KHNlbGYsIGN1cnJlbnRfcGlkKToKLSAgICAgICAgIiIiUmV0dXJuIFRydWUgaWYgcGlkIGlzIGFs aXZlLCBvdGhlcndpc2UgcmV0dXJuIEZhbHNlLiIiIgotICAgICAgICB0cnk6Ci0gICAgICAgICAg ICBvcy5raWxsKGN1cnJlbnRfcGlkLCAwKQotICAgICAgICBleGNlcHQgT1NFcnJvcjoKLSAgICAg ICAgICAgIHJldHVybiBGYWxzZQotICAgICAgICBlbHNlOgotICAgICAgICAgICAgcmV0dXJuIFRy dWUKKyAgICAgICAgIiIiUmV0dXJuIFRydWUgaWYgcGlkIGlzIGFsaXZlLCBvdGhlcndpc2UgcmV0 dXJuIEZhbHNlLgorICAgICAgICBGSVhNRTogb3Mua2lsbCgpIGRvZXNuJ3Qgd29yayBvbiBXaW5k b3dzIGZvciBjaGVja2luZyBpZgorICAgICAgICBhIHBpZCBpcyBhbGl2ZSwgc28gYWx3YXlzIHJl dHVybiBUcnVlIiIiCisgICAgICAgIGlmIHN5cy5wbGF0Zm9ybSBpbiAoJ2RhcndpbicsICdsaW51 eDInKToKKyAgICAgICAgICAgIHRyeToKKyAgICAgICAgICAgICAgICBvcy5raWxsKGN1cnJlbnRf cGlkLCAwKQorICAgICAgICAgICAgZXhjZXB0IE9TRXJyb3I6CisgICAgICAgICAgICAgICAgcmV0 dXJuIEZhbHNlCisgICAgICAgIHJldHVybiBUcnVlCiAKICAgICBkZWYgX2N1cmVudF9sb2NrX3Bp ZChzZWxmKToKICAgICAgICAgIiIiUmV0dXJuIHdpdGggdGhlIGN1cnJlbnQgbG9jayBwaWQuIElm IHRoZSBsb2NrIGlzIG5vdCB2YWxpZApAQCAtODksOSArOTEsNyBAQCBjbGFzcyBIdHRwTG9jayhv YmplY3QpOgogICAgICAgICAgICAgY3VycmVudF9sb2NrX2ZpbGUgPSBvcGVuKGxvY2tfbGlzdFsw XSwgJ3InKQogICAgICAgICAgICAgY3VycmVudF9waWQgPSBjdXJyZW50X2xvY2tfZmlsZS5yZWFk bGluZSgpCiAgICAgICAgICAgICBjdXJyZW50X2xvY2tfZmlsZS5jbG9zZSgpCi0gICAgICAgICAg ICBpZiBub3QgKGN1cnJlbnRfcGlkIGFuZAotICAgICAgICAgICAgICBzeXMucGxhdGZvcm0gaW4g KCdkYXJ3aW4nLCAnbGludXgyJykgYW5kCi0gICAgICAgICAgICAgIHNlbGYuX2NoZWNrX3BpZChp bnQoY3VycmVudF9waWQpKSk6CisgICAgICAgICAgICBpZiBub3QgKGN1cnJlbnRfcGlkIGFuZCBz ZWxmLl9jaGVja19waWQoaW50KGN1cnJlbnRfcGlkKSkpOgogICAgICAgICAgICAgICAgIG9zLnVu bGluayhsb2NrX2xpc3RbMF0pCiAgICAgICAgICAgICAgICAgcmV0dXJuCiAgICAgICAgIGV4Y2Vw dCBJT0Vycm9yLCBPU0Vycm9yOgpAQCAtMTA0LDkgKzEwNCw3IEBAIGNsYXNzIEh0dHBMb2NrKG9i amVjdCk6CiAgICAgICAgIG51bWJlcnMgYXJlIHNlcXVlbnRpYWwuIiIiCiAgICAgICAgIHdoaWxl KFRydWUpOgogICAgICAgICAgICAgdHJ5OgotICAgICAgICAgICAgICAgIHNlcXVlbnRpYWxfZ3Vh cmRfbG9jayA9IG9zLm9wZW4oc2VsZi5fZ3VhcmRfbG9ja19maWxlLAotICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgb3MuT19DUkVBVCB8IG9zLk9fTk9OQkxP Q0sgfCBvcy5PX0VYQ0wpCi0KKyAgICAgICAgICAgICAgICBzZXF1ZW50aWFsX2d1YXJkX2xvY2sg PSBvcy5vcGVuKHNlbGYuX2d1YXJkX2xvY2tfZmlsZSwgb3MuT19DUkVBVCB8IG9zLk9fRVhDTCkK ICAgICAgICAgICAgICAgICBzZWxmLl9wcm9jZXNzX2xvY2tfZmlsZV9uYW1lID0gKHNlbGYuX2xv Y2tfZmlsZV9wYXRoX3ByZWZpeCArCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICBzdHIoc2VsZi5fbmV4dF9sb2NrX251bWJlcigpKSkKICAgICAgICAgICAg ICAgICBsb2NrX2ZpbGUgPSBvcGVuKHNlbGYuX3Byb2Nlc3NfbG9ja19maWxlX25hbWUsICd3JykK