Bug 198851 - New EWS can't process patches larger than 640kb
Summary: New EWS can't process patches larger than 640kb
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
Keywords: InRadar
Depends on:
Blocks: 198834
  Show dependency treegraph
Reported: 2019-06-13 22:01 PDT by Daniel Bates
Modified: 2020-03-17 05:03 PDT (History)
5 users (show)

See Also:

Number 2 Twelve Hours Later! (84.85 KB, image/png)
2019-06-13 22:01 PDT, Daniel Bates
no flags Details
Patch (1.24 KB, patch)
2019-06-14 17:40 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (1.20 KB, patch)
2019-06-17 08:43 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-06-13 22:01:24 PDT
Created attachment 372104 [details]
Number 2 Twelve Hours Later!

I posted attachment #372061 [details] (bug #198834) at 10 am PDT today (06/13/2019). **12** hours later the patch is still listed as #2 on all New EWS queues! Old EWS long since processed the patch.
Comment 1 Radar WebKit Bug Importer 2019-06-13 22:03:50 PDT
Comment 2 Aakash Jain 2019-06-14 03:16:02 PDT
This one looks like some network issue (in communication between ews-app and buildbot) and probably some super corner case.

ews-app thinks it has sent the patch to Buildbot, but Buildbot never seems to have received it.

Logs from ews-app:

2019-06-13 10:00:35,320 - Logging in as ews-feeder@webkit.org...
2019-06-13 10:00:35,441 - Getting list of patches needing review, URL: https://bugs.webkit.org/request.cgi?action=queue&type=review&group=type
2019-06-13 10:00:36,357 - Saved patch in database, id: 372061 
2019-06-13 10:00:36,486 - 43 r? patches, 1 patches need to be sent to Buildbot.
2019-06-13 10:00:36,487 - Starting new HTTPS connection (1): bugs.webkit.org
2019-06-13 10:01:07,275 - Logging in as ews-feeder@webkit.org...

Patch is also present in /tmp/372061.patch (so there was no network/permission issue in downloading the patch).

Logs from twistd.log on Buildbot (ews-build):
2019-06-13 09:55:45-0700 [-] Splitting long line:     /Applications/Xcode.app/Co 6258 (not warning anymore for this log)
2019-06-13 09:56:40-0700 [-] Splitting long line:     /Applications/Xcode.app/Co 4444 (not warning anymore for this log)
2019-06-13 10:02:11-0700 [Broker,500,] user buildbot_try requesting build on builders None
2019-06-13 10:02:11-0700 [-] added buildset 9980 to database

No logs at 2019-06-13 10:00:36 (Note that buildset 9980 at 10:02:11 is for another patch). 

Will look into it more.
Comment 3 Aakash Jain 2019-06-14 04:10:28 PDT
I suspect this issue has something to do with large size of the patch (665 kb).
Comment 4 Aakash Jain 2019-06-14 04:56:39 PDT
Yes, the issue due to Patch size (over 640kb).

Manually running 'buildbot try' command results in below error. Also 'buildbot try' returns with exit code zero (even though it failed), that's why ews-app thinks it successful sent the patch to buildbot. Filed: https://github.com/buildbot/buildbot/issues/4837

Traceback (most recent call last):
  File "/usr/lib64/python2.7/site-packages/twisted/spread/pb.py", line 563, in expressionReceived
  File "/usr/lib64/python2.7/site-packages/twisted/spread/pb.py", line 933, in proto_answer
  File "/usr/lib64/python2.7/site-packages/twisted/internet/defer.py", line 393, in callback
  File "/usr/lib64/python2.7/site-packages/twisted/internet/defer.py", line 501, in _startRunCallbacks
--- <exception caught here> ---
  File "/usr/lib64/python2.7/site-packages/twisted/internet/defer.py", line 587, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
  File "/usr/lib/python2.7/site-packages/buildbot/clients/tryclient.py", line 702, in _deliverJob_pb
    self.config.get('properties', {}))
  File "/usr/lib64/python2.7/site-packages/twisted/spread/pb.py", line 343, in callRemote
    _name, args, kw)
  File "/usr/lib64/python2.7/site-packages/twisted/spread/pb.py", line 871, in _sendMessage
    self.sendCall(prefix+"message", requestID, objectID, message, answerRequired, netArgs, netKw)
  File "/usr/lib64/python2.7/site-packages/twisted/spread/pb.py", line 584, in sendCall
  File "/usr/lib64/python2.7/site-packages/twisted/spread/banana.py", line 320, in sendEncoded
    self._encode(obj, encodeStream.write)
  File "/usr/lib64/python2.7/site-packages/twisted/spread/banana.py", line 333, in _encode
    self._encode(elem, write)
  File "/usr/lib64/python2.7/site-packages/twisted/spread/banana.py", line 333, in _encode
    self._encode(elem, write)
  File "/usr/lib64/python2.7/site-packages/twisted/spread/banana.py", line 333, in _encode
    self._encode(elem, write)
  File "/usr/lib64/python2.7/site-packages/twisted/spread/banana.py", line 362, in _encode
    "byte string is too long to send (%d)" % (len(obj),))
twisted.spread.banana.BananaError: byte string is too long to send (681011)

[ews@ews ews-app]$ echo $?
Comment 5 Aakash Jain 2019-06-14 04:59:16 PDT
The error message comes from: https://github.com/twisted/twisted/blob/trunk/src/twisted/spread/banana.py#L364

And the limit is defined in: https://github.com/twisted/twisted/blob/trunk/src/twisted/spread/banana.py#L91

SIZE_LIMIT = 640 * 1024   # 640k is all you'll ever need :-)
Comment 6 Daniel Bates 2019-06-14 12:33:34 PDT
Fix, please?
Comment 7 Daniel Bates 2019-06-14 12:38:37 PDT
Oh, bug is upstream. That really sucks
Comment 8 Aakash Jain 2019-06-14 13:09:26 PDT
(In reply to Daniel Bates from comment #6)
> Fix, please?
Fix coming soon.
Comment 9 Aakash Jain 2019-06-14 17:40:46 PDT
Created attachment 372157 [details]
Comment 10 Aakash Jain 2019-06-14 17:42:54 PDT
(In reply to Aakash Jain from comment #9)
> Created attachment 372157 [details]
> Patch
This is similar to old fix in: <rdar://problem/33870706>.
Comment 11 Aakash Jain 2019-06-14 17:44:49 PDT
This is also as suggested in http://trac.buildbot.net/ticket/2123#comment:2
Comment 12 Alexey Proskuryakov 2019-06-14 22:44:41 PDT
5 MB seems too small as well, especially given that EWS gets into a strange state when seeming a large patch. 

I’m not sure what was the largest patch we had, but I won’t be surprised if it was over 100 MB.
Comment 13 Aakash Jain 2019-06-17 08:43:51 PDT
Created attachment 372247 [details]
Comment 14 Aakash Jain 2019-06-17 08:55:58 PDT
(In reply to Alexey Proskuryakov from comment #12)
> 5 MB seems too small as well, especially given that EWS gets into a strange state when seeming a large patch. 
> I’m not sure what was the largest patch we had, but I won’t be surprised if it was over 100 MB.
Increased the limit to 100 MB.

Also, this fix is at receiving end (buildbot). I need a similar fix on ews-app (django app) side, which invokes the 'buildbot try' command. I can't simply use this approach (importing banana and changing SIZE_LIMIT) in ews-app, since it invokes 'buildbot try' command using subprocess.call(). So I need a command-line/environment fix, instead of python fix. 

I will tackle it separately. For now, I have manually increased the limit on that machine (in /usr/lib64/python2.7/site-packages/twisted/spread/banana.py)
Comment 15 Jonathan Bedard 2019-06-17 08:58:03 PDT
I'd like to add some information coming from an IRC exchange Aakash and I had. According to https://github.com/twisted/twisted/blob/trunk/src/twisted/spread/banana.py#L364, our patches are basically being held as strings. This is why there is an upper limit. Ultimately, we aren't going to be able to get rid of the upper limit, and verging into the multi-gigabyte range could be problematic.

Ultimately, we just need to pay attention to this and bump the number again if we need to. We had an old version of patch-based EWS internally which used 2 MB, and I don't recall hearing any complaints, so I suspect that 100 MB will be fine.
Comment 16 WebKit Commit Bot 2019-06-17 09:31:43 PDT
Comment on attachment 372247 [details]

Clearing flags on attachment: 372247

Committed r246498: <https://trac.webkit.org/changeset/246498>
Comment 17 WebKit Commit Bot 2019-06-17 09:31:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Aakash Jain 2020-03-17 05:03:45 PDT
Largest patch I have seen in a while: 50MB in https://bugs.webkit.org/show_bug.cgi?id=209137