Mercurial > hg > mercurial-source

changeset 36773:6010fe1da619

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
wireprotoserver: document and improve the httplib workaround This workaround dates all the way back to a42d27bc809d in 2008. The code is esoteric enough to warrant an inline explanation. So I've added one. At the time the code was written, the only wire protocol command that accepted an HTTP request body was "unbundle." In the years since, we've grown the ability to accept command arguments via HTTP POST requests. So, the code has been changed to apply the httplib workaround to all HTTP POST requests. While staring at this code, I realized that the HTTP response body in case of error is always the same. And, it appears to mimic the behavior of a failed call to the "unbundle" command. Since we can hit this code path on theoretically any protocol request (since self.check_perm accepts custom auth checking functions which may raise), I'm having a hard time believing that clients react well to an "unbundle" response payload on any wire protocol command. I wouldn't be surprised if our test coverage for this feature only covers HTTP POST calls to "unbundle." In other words, the experimental support for sending arguments via HTTP POST request bodies may result in badness on the client. Something to investigate another time perhaps... Differential Revision: https://phab.mercurial-scm.org/D2064
author Gregory Szorc <gregory.szorc@gmail.com>
date Thu, 01 Feb 2018 16:11:54 -0800
parents 98a00aa0288d
children 04231e893a12
files mercurial/wireprotoserver.py
diffstat 1 files changed, 17 insertions(+), 4 deletions(-) [+]
line wrap: on
line diff
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -292,8 +292,9 @@
         req.respond(HTTP_OK, HGTYPE, body=rsp)
         return []
     elif isinstance(rsp, wireproto.pusherr):
-        # drain the incoming bundle
+        # This is the httplib workaround documented in _handlehttperror().
         req.drain()
+
         proto.restore()
         rsp = '0\n%s\n' % rsp.res
         req.respond(HTTP_OK, HGTYPE, body=rsp)
@@ -306,16 +307,28 @@
 
 def _handlehttperror(e, req, cmd):
     """Called when an ErrorResponse is raised during HTTP request processing."""
-    # A client that sends unbundle without 100-continue will
-    # break if we respond early.
-    if (cmd == 'unbundle' and
+
+    # Clients using Python's httplib are stateful: the HTTP client
+    # won't process an HTTP response until all request data is
+    # sent to the server. The intent of this code is to ensure
+    # we always read HTTP request data from the client, thus
+    # ensuring httplib transitions to a state that allows it to read
+    # the HTTP response. In other words, it helps prevent deadlocks
+    # on clients using httplib.
+
+    if (req.env[r'REQUEST_METHOD'] == r'POST' and
+        # But not if Expect: 100-continue is being used.
         (req.env.get('HTTP_EXPECT',
                      '').lower() != '100-continue') or
+        # Or the non-httplib HTTP library is being advertised by
+        # the client.
         req.env.get('X-HgHttp2', '')):
         req.drain()
     else:
         req.headers.append((r'Connection', r'Close'))
 
+    # TODO This response body assumes the failed command was
+    # "unbundle." That assumption is not always valid.
     req.respond(e, HGTYPE, body='0\n%s\n' % e)
 
     return ''