218994 2020-11-16 11:35:22 -0800 [ews] Add timeout to network requests 2020-11-18 14:51:20 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=214355 InRadar P2 Normal --- 1 aakash_jain aakash_jain aakash_jain ap dewei_zhu jbedard lingho ryanhaddad webkit-bug-importer oldest_to_newest 1707914 0 aakash_jain 2020-11-16 11:35:22 -0800 Sometimes network requests can get stuck or take very long time. It can introduce delays on EWS. Few days back (on nov 13), we noticed slowness on EWS when bugs.webkit.org requests were taking extremely long or getting stuck. We should add an explicit timeout to network requests, so that the network request is timely terminated. Also, as per https://requests.readthedocs.io/en/master/user/quickstart/#timeouts: "Nearly all production code should use this parameter in nearly all requests. Failure to do so can cause your program to hang indefinitely" 1707916 1 414261 aakash_jain 2020-11-16 11:38:08 -0800 Created attachment 414261 Patch 1707919 2 aakash_jain 2020-11-16 11:44:15 -0800 Note that the network requests failures in all these calls are gracefully handled. For e.g.: fetch_data_from_url_with_authentication() is used in get_patch_json/get_bug_json which are used in ValidatePatch step. In case of network request failure, validate-patch step will print a log about network request failure, mark the validate-patch step as warning, and continue with the subsequent build-steps. For load_contributors_from_trac() network request failure, it falls back to reading the contributors.json file from disk. For get_patch_status() network request failure in CheckPatchStatusOnEWSQueues step, commit-queue will continue processing the patch, although it might be inefficient, as it might run the layout-tests even if not strictly necessary. 1707928 3 lingho 2020-11-16 11:57:23 -0800 I think the timeout value to bugs.webkit.org should probably be set to a much lower value, like 2 seconds. Similarly for trac and ews-build connection, if you have't seen previous timeout, that should probably set to 2 seconds to make it useful. 1707953 4 414261 jbedard 2020-11-16 12:36:05 -0800 Comment on attachment 414261 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414261&action=review > Tools/CISupport/ews-build/steps.py:402 > + response = requests.get(url, timeout=15, params={'Bugzilla_api_key': self.get_bugzilla_api_key()}) I would second Ling's opinion that this timeout should be lower (I would say 5 seconds) This is the time to wait for any bytes, not the entire response: https://requests.readthedocs.io/en/master/user/quickstart/ 1707959 5 ap 2020-11-16 12:42:53 -0800 I'm not necessarily disagreeing, but can you explain why the timeout would be short? My instinct was to suggest making it at least 1 minute. Failing these requests could put the system in a bad state. 1707963 6 jbedard 2020-11-16 12:54:41 -0800 (In reply to Alexey Proskuryakov from comment #5) > I'm not necessarily disagreeing, but can you explain why the timeout would > be short? My instinct was to suggest making it at least 1 minute. > > Failing these requests could put the system in a bad state. My thinking is that if we can't receive any response (as in, not a single byte) from the server in 5 seconds, I wouldn't expect things to be different after 1 minute. That being said, I'm not sure that this change would even fix the original problem, was our issue that requests where not connecting to the server? The timeout option for requests doesn't apply to how long it takes to download the response, only to the time it takes for the server to start the response. 1707987 7 lingho 2020-11-16 14:06:36 -0800 I might have misunderstood the reason the timeouts are to be addded. I thought these timeouts are acceptable and will be handled accordingly. If the end goal is to not have a failed request, I guess there shouldn't even be a timeout specified. If a request eventually gets a timeout (not sure what's the default set up Python but I think the default Linux socket timeout is 60s), retry again and again until it eventually succeeded. As of why ews-build was affected as a whole by unavailable connection on bugs.webkit.org last week, the cause might need to be figured out and fixed differently. 1708752 8 414481 aakash_jain 2020-11-18 13:44:30 -0800 Created attachment 414481 Patch 1708767 9 aakash_jain 2020-11-18 14:02:30 -0800 Increased the timeout to 60s. It's not very clear why we saw performance issue on EWS on last Friday, but there were clearly many network requests which were stuck, taking extremely long etc. e.g.: in https://ews-build.webkit.org/#/builders/46/builds/2265 step #17 (validate-patch) took 2m:42s. Having 60s timeout is still much better than infinite timeout. Also, for most of these network requests, failure only make EWS build slightly inefficient (e.g.: validate-patch step being skipped causing EWS to process obsolete/r- patches). For load_contributors_from_trac(), it will read local copy from disk (which might be slightly old) 1708774 10 414481 jbedard 2020-11-18 14:20:03 -0800 Comment on attachment 414481 Patch I think nit-picking the value this should be is not a good use of our time, so let's land! 1708791 11 ews-feeder 2020-11-18 14:50:58 -0800 Committed r269992: <https://trac.webkit.org/changeset/269992> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414481. 1708793 12 webkit-bug-importer 2020-11-18 14:51:20 -0800 <rdar://problem/71556775> 414261 2020-11-16 11:38:08 -0800 2020-11-18 13:44:27 -0800 Patch bug-218994-20201116143807.patch text/plain 2956 aakash_jain SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDI2OTg2NSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE2IEBACisyMDIwLTExLTE2ICBBYWthc2ggSmFpbiAgPGFha2FzaF9qYWluQGFwcGxlLmNv bT4KKworICAgICAgICBbZXdzXSBBZGQgdGltZW91dCB0byBuZXR3b3JrIHJlcXVlc3RzCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTg5OTQKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIENJU3VwcG9ydC9l d3MtYnVpbGQvc3RlcHMucHk6CisgICAgICAgIChCdWd6aWxsYU1peGluLmZldGNoX2RhdGFfZnJv bV91cmxfd2l0aF9hdXRoZW50aWNhdGlvbik6IEFkZGVkIHRpbWVvdXQgdG8gcmVxdWVzdHMuZ2V0 KCkgY2FsbC4KKyAgICAgICAgKEJ1Z3ppbGxhTWl4aW4uZmV0Y2hfZGF0YV9mcm9tX3VybCk6IERp dHRvLgorICAgICAgICAoVmFsaWRhdGVDb21taXRlckFuZFJldmlld2VyLmxvYWRfY29udHJpYnV0 b3JzX2Zyb21fdHJhYyk6IERpdHRvLgorICAgICAgICAoQ2hlY2tQYXRjaFN0YXR1c09uRVdTUXVl dWVzLmdldF9wYXRjaF9zdGF0dXMpOiBEaXR0by4KKwogMjAyMC0xMS0xNiAgQ2hyaXMgRHVtZXog IDxjZHVtZXpAYXBwbGUuY29tPgogCiAgICAgICAgIEZpeCBzZXZlcmFsIGxlYWtzIG9mIENHQ29u dGV4dCBpbiBBUEkgdGVzdHMKSW5kZXg6IFRvb2xzL0NJU3VwcG9ydC9ld3MtYnVpbGQvc3RlcHMu cHkKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gVG9vbHMvQ0lTdXBwb3J0L2V3cy1idWlsZC9zdGVwcy5weQkocmV2 aXNpb24gMjY5ODY1KQorKysgVG9vbHMvQ0lTdXBwb3J0L2V3cy1idWlsZC9zdGVwcy5weQkod29y a2luZyBjb3B5KQpAQCAtMzk5LDcgKzM5OSw3IEBAIGNsYXNzIEJ1Z3ppbGxhTWl4aW4ob2JqZWN0 KToKICAgICBkZWYgZmV0Y2hfZGF0YV9mcm9tX3VybF93aXRoX2F1dGhlbnRpY2F0aW9uKHNlbGYs IHVybCk6CiAgICAgICAgIHJlc3BvbnNlID0gTm9uZQogICAgICAgICB0cnk6Ci0gICAgICAgICAg ICByZXNwb25zZSA9IHJlcXVlc3RzLmdldCh1cmwsIHBhcmFtcz17J0J1Z3ppbGxhX2FwaV9rZXkn OiBzZWxmLmdldF9idWd6aWxsYV9hcGlfa2V5KCl9KQorICAgICAgICAgICAgcmVzcG9uc2UgPSBy ZXF1ZXN0cy5nZXQodXJsLCB0aW1lb3V0PTE1LCBwYXJhbXM9eydCdWd6aWxsYV9hcGlfa2V5Jzog c2VsZi5nZXRfYnVnemlsbGFfYXBpX2tleSgpfSkKICAgICAgICAgICAgIGlmIHJlc3BvbnNlLnN0 YXR1c19jb2RlICE9IDIwMDoKICAgICAgICAgICAgICAgICBzZWxmLl9hZGRUb0xvZygnc3RkaW8n LCAnQWNjZXNzZWQge3VybH0gd2l0aCB1bmV4cGVjdGVkIHN0YXR1cyBjb2RlIHtzdGF0dXNfY29k ZX0uXG4nLmZvcm1hdCh1cmw9dXJsLCBzdGF0dXNfY29kZT1yZXNwb25zZS5zdGF0dXNfY29kZSkp CiAgICAgICAgICAgICAgICAgcmV0dXJuIE5vbmUKQEAgLTQxMiw3ICs0MTIsNyBAQCBjbGFzcyBC dWd6aWxsYU1peGluKG9iamVjdCk6CiAgICAgZGVmIGZldGNoX2RhdGFfZnJvbV91cmwoc2VsZiwg dXJsKToKICAgICAgICAgcmVzcG9uc2UgPSBOb25lCiAgICAgICAgIHRyeToKLSAgICAgICAgICAg IHJlc3BvbnNlID0gcmVxdWVzdHMuZ2V0KHVybCkKKyAgICAgICAgICAgIHJlc3BvbnNlID0gcmVx dWVzdHMuZ2V0KHVybCwgdGltZW91dD0xNSkKICAgICAgICAgZXhjZXB0IEV4Y2VwdGlvbiBhcyBl OgogICAgICAgICAgICAgaWYgcmVzcG9uc2U6CiAgICAgICAgICAgICAgICAgc2VsZi5fYWRkVG9M b2coJ3N0ZGlvJywgJ0ZhaWxlZCB0byBhY2Nlc3Mge3VybH0gd2l0aCBzdGF0dXMgY29kZSB7c3Rh dHVzX2NvZGV9LlxuJy5mb3JtYXQodXJsPXVybCwgc3RhdHVzX2NvZGU9cmVzcG9uc2Uuc3RhdHVz X2NvZGUpKQpAQCAtNzM5LDcgKzczOSw3IEBAIGNsYXNzIFZhbGlkYXRlQ29tbWl0ZXJBbmRSZXZp ZXdlcihidWlsZHMKIAogICAgIGRlZiBsb2FkX2NvbnRyaWJ1dG9yc19mcm9tX3RyYWMoc2VsZik6 CiAgICAgICAgIHRyeToKLSAgICAgICAgICAgIHJlc3BvbnNlID0gcmVxdWVzdHMuZ2V0KHNlbGYu dXJsKQorICAgICAgICAgICAgcmVzcG9uc2UgPSByZXF1ZXN0cy5nZXQoc2VsZi51cmwsIHRpbWVv dXQ9MTUpCiAgICAgICAgICAgICBpZiByZXNwb25zZS5zdGF0dXNfY29kZSAhPSAyMDA6CiAgICAg ICAgICAgICAgICAgc2VsZi5fYWRkVG9Mb2coJ3N0ZGlvJywgJ0ZhaWxlZCB0byBhY2Nlc3Mge30g d2l0aCBzdGF0dXMgY29kZToge31cbicuZm9ybWF0KHNlbGYudXJsLCByZXNwb25zZS5zdGF0dXNf Y29kZSkpCiAgICAgICAgICAgICAgICAgcmV0dXJuIHt9CkBAIC0zMDUzLDcgKzMwNTMsNyBAQCBj bGFzcyBDaGVja1BhdGNoU3RhdHVzT25FV1NRdWV1ZXMoYnVpbGRzCiAgICAgZGVmIGdldF9wYXRj aF9zdGF0dXMoc2VsZiwgcGF0Y2hfaWQsIHF1ZXVlKToKICAgICAgICAgdXJsID0gJ3t9c3RhdHVz L3t9Jy5mb3JtYXQoRVdTX1VSTCwgcGF0Y2hfaWQpCiAgICAgICAgIHRyeToKLSAgICAgICAgICAg IHJlc3BvbnNlID0gcmVxdWVzdHMuZ2V0KHVybCkKKyAgICAgICAgICAgIHJlc3BvbnNlID0gcmVx dWVzdHMuZ2V0KHVybCwgdGltZW91dD0xNSkKICAgICAgICAgICAgIGlmIHJlc3BvbnNlLnN0YXR1 c19jb2RlICE9IDIwMDoKICAgICAgICAgICAgICAgICBzZWxmLl9hZGRUb0xvZygnc3RkaW8nLCAn RmFpbGVkIHRvIGFjY2VzcyB7fSB3aXRoIHN0YXR1cyBjb2RlOiB7fVxuJy5mb3JtYXQodXJsLCBy ZXNwb25zZS5zdGF0dXNfY29kZSkpCiAgICAgICAgICAgICAgICAgcmV0dXJuIC0xCg== 414481 2020-11-18 13:44:30 -0800 2020-11-18 14:50:59 -0800 Patch bug-218994-20201118164429.patch text/plain 2956 aakash_jain SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDI2OTg2NSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE2IEBACisyMDIwLTExLTE2ICBBYWthc2ggSmFpbiAgPGFha2FzaF9qYWluQGFwcGxlLmNv bT4KKworICAgICAgICBbZXdzXSBBZGQgdGltZW91dCB0byBuZXR3b3JrIHJlcXVlc3RzCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTg5OTQKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIENJU3VwcG9ydC9l d3MtYnVpbGQvc3RlcHMucHk6CisgICAgICAgIChCdWd6aWxsYU1peGluLmZldGNoX2RhdGFfZnJv bV91cmxfd2l0aF9hdXRoZW50aWNhdGlvbik6IEFkZGVkIHRpbWVvdXQgdG8gcmVxdWVzdHMuZ2V0 KCkgY2FsbC4KKyAgICAgICAgKEJ1Z3ppbGxhTWl4aW4uZmV0Y2hfZGF0YV9mcm9tX3VybCk6IERp dHRvLgorICAgICAgICAoVmFsaWRhdGVDb21taXRlckFuZFJldmlld2VyLmxvYWRfY29udHJpYnV0 b3JzX2Zyb21fdHJhYyk6IERpdHRvLgorICAgICAgICAoQ2hlY2tQYXRjaFN0YXR1c09uRVdTUXVl dWVzLmdldF9wYXRjaF9zdGF0dXMpOiBEaXR0by4KKwogMjAyMC0xMS0xNiAgQ2hyaXMgRHVtZXog IDxjZHVtZXpAYXBwbGUuY29tPgogCiAgICAgICAgIEZpeCBzZXZlcmFsIGxlYWtzIG9mIENHQ29u dGV4dCBpbiBBUEkgdGVzdHMKSW5kZXg6IFRvb2xzL0NJU3VwcG9ydC9ld3MtYnVpbGQvc3RlcHMu cHkKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gVG9vbHMvQ0lTdXBwb3J0L2V3cy1idWlsZC9zdGVwcy5weQkocmV2 aXNpb24gMjY5OTAxKQorKysgVG9vbHMvQ0lTdXBwb3J0L2V3cy1idWlsZC9zdGVwcy5weQkod29y a2luZyBjb3B5KQpAQCAtMzk5LDcgKzM5OSw3IEBAIGNsYXNzIEJ1Z3ppbGxhTWl4aW4ob2JqZWN0 KToKICAgICBkZWYgZmV0Y2hfZGF0YV9mcm9tX3VybF93aXRoX2F1dGhlbnRpY2F0aW9uKHNlbGYs IHVybCk6CiAgICAgICAgIHJlc3BvbnNlID0gTm9uZQogICAgICAgICB0cnk6Ci0gICAgICAgICAg ICByZXNwb25zZSA9IHJlcXVlc3RzLmdldCh1cmwsIHBhcmFtcz17J0J1Z3ppbGxhX2FwaV9rZXkn OiBzZWxmLmdldF9idWd6aWxsYV9hcGlfa2V5KCl9KQorICAgICAgICAgICAgcmVzcG9uc2UgPSBy ZXF1ZXN0cy5nZXQodXJsLCB0aW1lb3V0PTYwLCBwYXJhbXM9eydCdWd6aWxsYV9hcGlfa2V5Jzog c2VsZi5nZXRfYnVnemlsbGFfYXBpX2tleSgpfSkKICAgICAgICAgICAgIGlmIHJlc3BvbnNlLnN0 YXR1c19jb2RlICE9IDIwMDoKICAgICAgICAgICAgICAgICBzZWxmLl9hZGRUb0xvZygnc3RkaW8n LCAnQWNjZXNzZWQge3VybH0gd2l0aCB1bmV4cGVjdGVkIHN0YXR1cyBjb2RlIHtzdGF0dXNfY29k ZX0uXG4nLmZvcm1hdCh1cmw9dXJsLCBzdGF0dXNfY29kZT1yZXNwb25zZS5zdGF0dXNfY29kZSkp CiAgICAgICAgICAgICAgICAgcmV0dXJuIE5vbmUKQEAgLTQxMiw3ICs0MTIsNyBAQCBjbGFzcyBC dWd6aWxsYU1peGluKG9iamVjdCk6CiAgICAgZGVmIGZldGNoX2RhdGFfZnJvbV91cmwoc2VsZiwg dXJsKToKICAgICAgICAgcmVzcG9uc2UgPSBOb25lCiAgICAgICAgIHRyeToKLSAgICAgICAgICAg IHJlc3BvbnNlID0gcmVxdWVzdHMuZ2V0KHVybCkKKyAgICAgICAgICAgIHJlc3BvbnNlID0gcmVx dWVzdHMuZ2V0KHVybCwgdGltZW91dD02MCkKICAgICAgICAgZXhjZXB0IEV4Y2VwdGlvbiBhcyBl OgogICAgICAgICAgICAgaWYgcmVzcG9uc2U6CiAgICAgICAgICAgICAgICAgc2VsZi5fYWRkVG9M b2coJ3N0ZGlvJywgJ0ZhaWxlZCB0byBhY2Nlc3Mge3VybH0gd2l0aCBzdGF0dXMgY29kZSB7c3Rh dHVzX2NvZGV9LlxuJy5mb3JtYXQodXJsPXVybCwgc3RhdHVzX2NvZGU9cmVzcG9uc2Uuc3RhdHVz X2NvZGUpKQpAQCAtNzM5LDcgKzczOSw3IEBAIGNsYXNzIFZhbGlkYXRlQ29tbWl0ZXJBbmRSZXZp ZXdlcihidWlsZHMKIAogICAgIGRlZiBsb2FkX2NvbnRyaWJ1dG9yc19mcm9tX3RyYWMoc2VsZik6 CiAgICAgICAgIHRyeToKLSAgICAgICAgICAgIHJlc3BvbnNlID0gcmVxdWVzdHMuZ2V0KHNlbGYu dXJsKQorICAgICAgICAgICAgcmVzcG9uc2UgPSByZXF1ZXN0cy5nZXQoc2VsZi51cmwsIHRpbWVv dXQ9NjApCiAgICAgICAgICAgICBpZiByZXNwb25zZS5zdGF0dXNfY29kZSAhPSAyMDA6CiAgICAg ICAgICAgICAgICAgc2VsZi5fYWRkVG9Mb2coJ3N0ZGlvJywgJ0ZhaWxlZCB0byBhY2Nlc3Mge30g d2l0aCBzdGF0dXMgY29kZToge31cbicuZm9ybWF0KHNlbGYudXJsLCByZXNwb25zZS5zdGF0dXNf Y29kZSkpCiAgICAgICAgICAgICAgICAgcmV0dXJuIHt9CkBAIC0zMDUzLDcgKzMwNTMsNyBAQCBj bGFzcyBDaGVja1BhdGNoU3RhdHVzT25FV1NRdWV1ZXMoYnVpbGRzCiAgICAgZGVmIGdldF9wYXRj aF9zdGF0dXMoc2VsZiwgcGF0Y2hfaWQsIHF1ZXVlKToKICAgICAgICAgdXJsID0gJ3t9c3RhdHVz L3t9Jy5mb3JtYXQoRVdTX1VSTCwgcGF0Y2hfaWQpCiAgICAgICAgIHRyeToKLSAgICAgICAgICAg IHJlc3BvbnNlID0gcmVxdWVzdHMuZ2V0KHVybCkKKyAgICAgICAgICAgIHJlc3BvbnNlID0gcmVx dWVzdHMuZ2V0KHVybCwgdGltZW91dD02MCkKICAgICAgICAgICAgIGlmIHJlc3BvbnNlLnN0YXR1 c19jb2RlICE9IDIwMDoKICAgICAgICAgICAgICAgICBzZWxmLl9hZGRUb0xvZygnc3RkaW8nLCAn RmFpbGVkIHRvIGFjY2VzcyB7fSB3aXRoIHN0YXR1cyBjb2RlOiB7fVxuJy5mb3JtYXQodXJsLCBy ZXNwb25zZS5zdGF0dXNfY29kZSkpCiAgICAgICAgICAgICAgICAgcmV0dXJuIC0xCg==