Bug 198851 – New EWS can't process patches larger than 640kb

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

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.

[Radar WebKit Bug Importer]

<rdar://problem/51735036>

[Aakash Jain]

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 [e-mail]...
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 [e-mail]...

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,172.31.50.61] 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.

[Aakash Jain]

I suspect this issue has something to do with large size of the patch (665 kb).

[Aakash Jain]

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
    method(*sexp[1:])
  File "/usr/lib64/python2.7/site-packages/twisted/spread/pb.py", line 933, in proto_answer
    d.callback(self.unserialize(netResult))
  File "/usr/lib64/python2.7/site-packages/twisted/internet/defer.py", line 393, in callback
    self._startRunCallbacks(result)
  File "/usr/lib64/python2.7/site-packages/twisted/internet/defer.py", line 501, in _startRunCallbacks
    self._runCallbacks()
--- <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
    self.sendEncoded(exp)
  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 $?
0

[Aakash Jain]

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 :-)

[Daniel Bates]

Fix, please?

[Daniel Bates]

Oh, bug is upstream. That really sucks

[Aakash Jain]

(In reply to Daniel Bates from comment #6)
> Fix, please?
Fix coming soon.

[Aakash Jain]

Created attachment 372157 [details]
Patch

[Aakash Jain]

(In reply to Aakash Jain from comment #9)
> Created attachment 372157 [details]
> Patch
This is similar to old fix in: <rdar://problem/33870706>.

[Aakash Jain]

This is also as suggested in http://trac.buildbot.net/ticket/2123#comment:2

[Alexey Proskuryakov]

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.

[Aakash Jain]

Created attachment 372247 [details]
Patch

[Aakash Jain]

(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)

[Jonathan Bedard]

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.

[WebKit Commit Bot]

Comment on attachment 372247 [details]
Patch

Clearing flags on attachment: 372247

Committed r246498: <https://trac.webkit.org/changeset/246498>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

Largest patch I have seen in a while: 50MB in https://bugs.webkit.org/show_bug.cgi?id=209137