171463 2017-04-28 16:47:42 -0700 EWS QueueServer shouldn't fail to release patch if last_status is None 2018-06-22 17:41:04 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Local Build Unspecified Unspecified RESOLVED DUPLICATE 186748 P2 Normal --- 1 aakash_jain aakash_jain aakash_jain ap dbates dewei_zhu lforschler oldest_to_newest 1302869 0 aakash_jain 2017-04-28 16:47:42 -0700 Noticed following error in logs: ERROR 2017-04-28 16:34:57,859 cgi.py:121] Traceback (most recent call last): File "/var/apps/webkit-queues/app/main.py", line 90, in main run_wsgi_app(application) File "/root/appscale/AppServer/google/appengine/ext/webapp/util.py", line 98, in run_wsgi_app run_bare_wsgi_app(add_wsgi_middleware(application)) File "/root/appscale/AppServer/google/appengine/ext/webapp/util.py", line 116, in run_bare_wsgi_app result = application(env, _start_response) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1519, in __call__ response = self._internal_error(e) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1511, in __call__ rv = self.handle_exception(request, response, e) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1505, in __call__ rv = self.router.dispatch(request, response) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1253, in default_dispatcher return route.handler_adapter(request, response) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 1077, in __call__ return handler.dispatch() File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 547, in dispatch return self.handle_exception(e, self.app.debug) File "/root/appscale/AppServer/lib/webapp2-2.3/webapp2.py", line 545, in dispatch return method(*args, **kwargs) File "/var/apps/webkit-queues/app/handlers/releasepatch.py", line 58, in post RecordPatchEvent.stopped(attachment_id, queue_name, last_status.message) AttributeError: 'NoneType' object has no attribute 'message' 1302870 1 308615 aakash_jain 2017-04-28 16:51:07 -0700 Created attachment 308615 Proposed patch 1303574 2 308615 ap 2017-05-01 21:27:50 -0700 Comment on attachment 308615 Proposed patch Why is this message being recorded by 61 RecordPatchEvent.stopped in the first place? I'm trying to understand what problems code down the line could hit. 1303864 3 aakash_jain 2017-05-02 15:57:18 -0700 The exception happened while trying to access "last_status.message", while last_status was None. I believe last_status was None, because the slave machine, for some reason tried to release the patch immediately after starting it (See <rdar://problem/31894087>). So, there was no last_status for this patch on the server. And so accessing last_status.message causes exception. This patch simply check if last_status is None and handles it. 1303879 4 ap 2017-05-02 16:26:20 -0700 With the fix, we will be recording an event with an empty message. Where is this message used? Is it OK to have an empty one? 1303913 5 aakash_jain 2017-05-02 18:30:05 -0700 I tried to test this change with different message (instead of empty) and couldn't find that message anywhere (atleast not in UI). This probably just go in the database. last_status being None seems like a major problem with the patch. I believe, it happens when the patch is not processed at all (and directly released), possibly because ews was not able to parse the bug id properly. e.g.: https://bugs.webkit.org/show_bug.cgi?id=170094 If you prefer, instead of empty message, we can keep the message something like "Unexpected error". 1304043 6 ap 2017-05-03 09:19:03 -0700 I don't have a preference for what message to write without knowing where it goes. If it doesn't go anywhere, can we just remove it from all calls to stopped()? 1364336 7 ap 2017-10-25 11:28:38 -0700 Assuming that the problem is still happening, what's the right way to fix it? Should we go with this patch, or stop attempting to produce this message? 1364348 8 aakash_jain 2017-10-25 11:39:33 -0700 This is a corner case, this should still be happening. This happens when EWS process a bug which which have another bug id in the bug title (e.g.: https://bugs.webkit.org/show_bug.cgi?id=170094). See <rdar://problem/31894087>. I would still like to land this patch as is. I think this is definitely an improvement. 1364475 9 ap 2017-10-25 14:25:00 -0700 I think that I'd still like to understand where the message is visible to the user to meaningfully review. 1435713 10 308615 dbates 2018-06-22 17:40:53 -0700 Comment on attachment 308615 Proposed patch last_status.message is None if an EWS bot chose to skip a patch before it committed to take it (see AbstractPatchQueue._next_patch()). The EWS bot will skip the patch if it failed to fetch it from Bugzilla (say, Bugzilla was down or returned a bad response). When this happens the EWS bot does not post a status update that it skipped the patch before it tells the status server to release the patch. Instead the EWS should post a status update that it has skipped the patch like it does when it skips a patch that it committed to processing. See the ChaneLog entry in the patch on bug #186748 for more details. 1435714 11 dbates 2018-06-22 17:41:04 -0700 *** This bug has been marked as a duplicate of bug 186748 *** 308615 2017-04-28 16:51:07 -0700 2018-06-22 17:40:53 -0700 Proposed patch patch_ews_last_status_null text/plain 1613 aakash_jain SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDIxNTk1NykKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDEzIEBACisyMDE3LTA0LTI4ICBBYWthc2ggSmFpbiAgPGFha2FzaF9qYWluQGFwcGxlLmNv bT4KKworICAgICAgICBFV1MgUXVldWVTZXJ2ZXIgc2hvdWxkbid0IGZhaWwgdG8gcmVsZWFzZSBw YXRjaCBpZiBsYXN0X3N0YXR1cyBpcyBOb25lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xNzE0NjMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICAqIFF1ZXVlU3RhdHVzU2VydmVyL2hhbmRsZXJzL3JlbGVhc2Vw YXRjaC5weToKKyAgICAgICAgKFJlbGVhc2VQYXRjaC5wb3N0KTogQWNjZXNzIGxhc3Rfc3RhdHVz Lm1lc3NhZ2Ugb25seSBpZiBsYXN0X3N0YXR1cyBpcyBub3QgTm9uZS4KKwogMjAxNy0wNC0yOCAg Sm9uYXRoYW4gQmVkYXJkICA8amJlZGFyZEBhcHBsZS5jb20+CiAKICAgICAgICAgVW5yZXZpZXdl ZCBmb2xsb3ctdXAgdG8gcjIxNDcwNS4KSW5kZXg6IFRvb2xzL1F1ZXVlU3RhdHVzU2VydmVyL2hh bmRsZXJzL3JlbGVhc2VwYXRjaC5weQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9RdWV1ZVN0YXR1c1Nl cnZlci9oYW5kbGVycy9yZWxlYXNlcGF0Y2gucHkJKHJldmlzaW9uIDIxNTk1NykKKysrIFRvb2xz L1F1ZXVlU3RhdHVzU2VydmVyL2hhbmRsZXJzL3JlbGVhc2VwYXRjaC5weQkod29ya2luZyBjb3B5 KQpAQCAtNTAsMTEgKzUwLDE0IEBAIGNsYXNzIFJlbGVhc2VQYXRjaChVcGRhdGVCYXNlKToKICAg ICAgICAgYXR0YWNobWVudF9pZCA9IHNlbGYuX2ludF9mcm9tX3JlcXVlc3QoImF0dGFjaG1lbnRf aWQiKQogICAgICAgICBhdHRhY2htZW50ID0gQXR0YWNobWVudChhdHRhY2htZW50X2lkKQogICAg ICAgICBsYXN0X3N0YXR1cyA9IGF0dGFjaG1lbnQuc3RhdHVzX2Zvcl9xdWV1ZShxdWV1ZSkKKyAg ICAgICAgbWVzc2FnZSA9ICIiCisgICAgICAgIGlmIGxhc3Rfc3RhdHVzOgorICAgICAgICAgICAg bWVzc2FnZSA9IGxhc3Rfc3RhdHVzLm1lc3NhZ2UKIAogICAgICAgICAjIElkZWFsbHkgd2Ugc2hv dWxkIHVzZSBhIHRyYW5zYWN0aW9uIGZvciB0aGUgY2FsbHMgdG8KICAgICAgICAgIyBXb3JrSXRl bXMgYW5kIEFjdGl2ZVdvcmtJdGVtcy4KIAogICAgICAgICBxdWV1ZS53b3JrX2l0ZW1zKCkucmVt b3ZlX3dvcmtfaXRlbShhdHRhY2htZW50X2lkKQotICAgICAgICBSZWNvcmRQYXRjaEV2ZW50LnN0 b3BwZWQoYXR0YWNobWVudF9pZCwgcXVldWVfbmFtZSwgbGFzdF9zdGF0dXMubWVzc2FnZSkKKyAg ICAgICAgUmVjb3JkUGF0Y2hFdmVudC5zdG9wcGVkKGF0dGFjaG1lbnRfaWQsIHF1ZXVlX25hbWUs IG1lc3NhZ2UpCiAKICAgICAgICAgcXVldWUuYWN0aXZlX3dvcmtfaXRlbXMoKS5leHBpcmVfaXRl bShhdHRhY2htZW50X2lkKQo=