Patchwork [Bug,5646] New: zstd-compressed bundles don't work over the ssh protocol

login
register
mail settings
Submitter mercurial-bugs@mercurial-scm.org
Date Aug. 1, 2017, 3:41 a.m.
Message ID <bug-5646-285@https.bz.mercurial-scm.org/>
Download mbox | patch
Permalink /patch/22598/
State Not Applicable
Headers show

Comments

mercurial-bugs@mercurial-scm.org - Aug. 1, 2017, 3:41 a.m.
https://bz.mercurial-scm.org/show_bug.cgi?id=5646

            Bug ID: 5646
           Summary: zstd-compressed bundles don't work over the ssh
                    protocol
           Product: Mercurial
           Version: unspecified
          Hardware: All
                OS: All
            Status: UNCONFIRMED
          Severity: bug
          Priority: normal
         Component: Mercurial
          Assignee: bugzilla@mercurial-scm.org
          Reporter: sid+hgbz@less-broken.com
                CC: gregory.szorc@gmail.com,
                    mercurial-devel@mercurial-scm.org

With this patch applied:



an hg pull over ssh hangs with this stack trace:
https://bpaste.net/show/6e8ad89b79e6

The 'size' at line 40 of the stack trace is 131075. This appears to be
something inside zstd requesting 128KiB chunks.

This causes the os.read(..., _chunksize) at line 46 to hang, because that
method doesn't return unless _chunksize (= 4096 in this case) bytes are read
off the wire.

This isn't an issue with stock Mercurial because it always sends uncompressed
bundles over the wire.

Some IRC discussion:

14:49 <indygreg> sid0: iirc it buffers the wire up to 128kb then feeds that
into the decompressor
14:49 <sid0> indygreg: what if there are less than 128kb available?
14:49 <indygreg> i'd have to look at the code. it either tries again or runs
with what it's got
14:49 <sid0> indygreg: (also this behavior doesn't happen with uncompressed
bundles)
14:50 <sid0> actually it hangs :)
14:51 <indygreg> hanging in util.chunkbuffer?
14:55 <sid0> indygreg: https://bpaste.net/show/66f17d9abf36
14:55 <sid0> indygreg: the "size" in the read call at line 40 is 131075
14:56 <indygreg> welcome to the hell that is the buffering code for bundle
reading
14:57 <indygreg> why is os.read() being used?
14:57 <indygreg> i think that will hang until all requested data is available
14:58 <indygreg> that's the wrong function to be used where you can't guarantee
other end will produce that much data
14:58 <sid0> indygreg: hey, this is the first time I've seen this code :)
14:58 <indygreg> same here
14:58 <indygreg> this code is pretty dangerous
14:58 <indygreg> i'm mildly surprised others aren't complaining about deadlocks
14:59 <sid0> indygreg: I did have to trigger this by patching the code to send
zstd-compressed bundles over the wire
14:59 <sid0> indygreg: but yeah
14:59 <sid0> indygreg: so if not os.read, what should it be using instead?
15:00 <indygreg> well, it depends if O_NONBLOCK was set
15:01 <indygreg> as long as the file descriptor isn't in blocking mode, it
should be OK
15:01 <indygreg> but since bufferedinputpipe() doesn't control the socket, ugh
15:01 <indygreg> err, file descriptor
15:01 <indygreg> also, i think python may retry automatically in some cases
15:01 <indygreg> i'd have to look at cpython source
15:02 <indygreg> this feels fishy to me

Patch

diff --git mercurial/bundle2.py mercurial/bundle2.py
--- mercurial/bundle2.py
+++ mercurial/bundle2.py
@@ -542,7 +542,8 @@  class bundle20(object):
         self._params = []
         self._parts = []
         self.capabilities = dict(capabilities)
-        self._compengine = util.compengines.forbundletype('UN')
+        self._compengine = util.compengines.forbundletype('ZS')
+        self.addparam('Compression', 'ZS')
         self._compopts = None

     def setcompression(self, alg, compopts=None):