Patchwork D2721: util: observable proxy objects for sockets

login
register
mail settings
Submitter phabricator
Date March 8, 2018, 5:31 a.m.
Message ID <differential-rev-PHID-DREV-lpjmvoomcirf27hhf47i-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29119/
State Superseded
Headers show

Comments

phabricator - March 8, 2018, 5:31 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We previously introduced proxy objects and observers for file objects
  to help implement low-level tests for the SSH wire protocol.
  
  In this commit, we do the same for sockets in order to help test the
  HTTP server.
  
  We only proxy/observe some socket methods. I didn't feel like
  implementing all the methods because there are so many of them and
  implementing them will provide no short term value. We can always
  implement them later.
  
  1. no-check-commit because we implement foo_bar methods on stdlib types

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2721

AFFECTED FILES
  mercurial/util.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - March 13, 2018, 5:38 p.m.
indygreg planned changes to this revision.
indygreg added a comment.


  I have significant changes to this series in flight. It's worth holding off on review.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2721

To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - March 13, 2018, 7:21 p.m.
indygreg requested review of this revision.
indygreg added a comment.


  This series should now be ready to review.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2721

To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - March 13, 2018, 9:11 p.m.
mharbison72 added a comment.


  fileobjectproxy needed `__nonzero__()`/`__bool__()`.  Does socketproxy need it for consistency?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2721

To: indygreg, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - May 20, 2018, 9:08 a.m.
pulkit added inline comments.

INLINE COMMENTS

> util.py:1019
> +        self.fh.write('%s> setsockopt(%r, %r, %r) -> %r\n' % (
> +            self.name, level, optname, value))
> +

If I am understanding this correctly, this can lead to `TypeError: not enough arguments for format string`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2721

To: indygreg, #hg-reviewers, durin42
Cc: pulkit, mharbison72, mercurial-devel
phabricator - May 20, 2018, 9:28 a.m.
pulkit added inline comments.

INLINE COMMENTS

> util.py:1014
> +
> +    def setsockopt(self, level, optname, value):
> +        if not self.states:

While debugging the test failure on Python 3.6, the caller at line 751 can pass 5 positional arguments.

Here is the traceback after running test-wireproto-command-branchmap.t:

  +  ** Unknown exception encountered with possibly-broken third-party extension drawdag
  +  ** which supports versions unknown of Mercurial.
  +  ** Please disable drawdag and try your action again.
  +  ** If that fixes the bug please report it to the extension author.
  +  ** Python 3.6.5 (default, Mar 29 2018, 03:28:50) [GCC 5.4.0 20160609]
  +  ** Mercurial Distributed SCM (version 4.6+251-5a87bf0bd343)
  +  ** Extensions loaded: drawdag
  +  Traceback (most recent call last):
  +    File "/tmp/hgtests.esettwc6/install/bin/hg", line 41, in <module>
  +      dispatch.run()
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 90, in run
  +      status = dispatch(req)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 213, in dispatch
  +      ret = _runcatch(req) or 0
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 354, in _runcatch
  +      return _callcatch(ui, _runcatchfunc)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 362, in _callcatch
  +      return scmutil.callcatch(ui, func)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/scmutil.py", line 161, in callcatch
  +      return func()
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 344, in _runcatchfunc
  +      return _dispatch(req)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 974, in _dispatch
  +      cmdpats, cmdoptions)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 730, in runcommand
  +      ret = _runcommand(ui, options, cmd, d)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 982, in _runcommand
  +      return cmdfunc()
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/dispatch.py", line 971, in <lambda>
  +      d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/util.py", line 1550, in check
  +      return func(*args, **kwargs)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/debugcommands.py", line 2932, in debugwireproto
  +      peer = httppeer.makepeer(ui, path, opener=opener)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/httppeer.py", line 951, in makepeer
  +      respurl, info = performhandshake(ui, url, opener, requestbuilder)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/httppeer.py", line 871, in performhandshake
  +      resp = sendrequest(ui, opener, req)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/httppeer.py", line 311, in sendrequest
  +      res = opener.open(req)
  +    File "/usr/lib/python3.6/urllib/request.py", line 526, in open
  +      response = self._open(req, data)
  +    File "/usr/lib/python3.6/urllib/request.py", line 544, in _open
  +      '_open', req)
  +    File "/usr/lib/python3.6/urllib/request.py", line 504, in _call_chain
  +      result = func(*args)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/url.py", line 331, in http_open
  +      return self.do_open(self._makeconnection, req)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/keepalive.py", line 240, in do_open
  +      self._start_transaction(h, req)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/url.py", line 301, in _start_transaction
  +      return keepalive.HTTPHandler._start_transaction(self, h, req)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/keepalive.py", line 343, in _start_transaction
  +      h.endheaders()
  +    File "/usr/lib/python3.6/http/client.py", line 1234, in endheaders
  +      self._send_output(message_body, encode_chunked=encode_chunked)
  +    File "/usr/lib/python3.6/http/client.py", line 1026, in _send_output
  +      self.send(msg)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/keepalive.py", line 563, in safesend
  +      self.connect()
  +    File "/usr/lib/python3.6/http/client.py", line 937, in connect
  +      self.sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/util.py", line 738, in setsockopt
  +      r'setsockopt', *args, **kwargs)
  +    File "/tmp/hgtests.esettwc6/install/lib/python/mercurial/util.py", line 679, in _observedcall
  +      fn(res, *args, **kwargs)
  +  TypeError: setsockopt() takes 4 positional arguments but 5 were given
  +  [1]

I am not sure whether this exact code path is tested on Python 2 or not. Can you have a look?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2721

To: indygreg, #hg-reviewers, durin42
Cc: pulkit, mharbison72, mercurial-devel

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -695,6 +695,120 @@ 
 
         return res
 
+PROXIED_SOCKET_METHODS = {
+    r'makefile',
+    r'recv',
+    r'recvfrom',
+    r'recvfrom_into',
+    r'recv_into',
+    r'send',
+    r'sendall',
+    r'sendto',
+    r'setblocking',
+    r'settimeout',
+    r'gettimeout',
+    r'setsockopt',
+}
+
+class socketproxy(object):
+    """A proxy around a socket that tells a watcher when events occur.
+
+    This is like ``fileobjectproxy`` except for sockets.
+
+    This type is intended to only be used for testing purposes. Think hard
+    before using it in important code.
+    """
+    __slots__ = (
+        r'_orig',
+        r'_observer',
+    )
+
+    def __init__(self, sock, observer):
+        object.__setattr__(self, r'_orig', sock)
+        object.__setattr__(self, r'_observer', observer)
+
+    def __getattribute__(self, name):
+        if name in PROXIED_SOCKET_METHODS:
+            return object.__getattribute__(self, name)
+
+        return getattr(object.__getattribute__(self, r'_orig'), name)
+
+    def __delattr__(self, name):
+        return delattr(object.__getattribute__(self, r'_orig'), name)
+
+    def __setattr__(self, name, value):
+        return setattr(object.__getattribute__(self, r'_orig'), name, value)
+
+    def _observedcall(self, name, *args, **kwargs):
+        # Call the original object.
+        orig = object.__getattribute__(self, r'_orig')
+        res = getattr(orig, name)(*args, **kwargs)
+
+        # Call a method on the observer of the same name with arguments
+        # so it can react, log, etc.
+        observer = object.__getattribute__(self, r'_observer')
+        fn = getattr(observer, name, None)
+        if fn:
+            fn(res, *args, **kwargs)
+
+        return res
+
+    def makefile(self, *args, **kwargs):
+        res = object.__getattribute__(self, r'_observedcall')(
+            r'makefile', *args, **kwargs)
+
+        # The file object may be used for I/O. So we turn it into a
+        # proxy using our observer.
+        observer = object.__getattribute__(self, r'_observer')
+        return makeloggingfileobject(observer.fh, res, observer.name,
+                                     reads=observer.reads,
+                                     writes=observer.writes,
+                                     logdata=observer.logdata)
+
+    def recv(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'recv', *args, **kwargs)
+
+    def recvfrom(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'recvfrom', *args, **kwargs)
+
+    def recvfrom_into(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'recvfrom_into', *args, **kwargs)
+
+    def recv_into(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'recv_info', *args, **kwargs)
+
+    def send(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'send', *args, **kwargs)
+
+    def sendall(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'sendall', *args, **kwargs)
+
+    def sendto(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'sendto', *args, **kwargs)
+
+    def setblocking(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'setblocking', *args, **kwargs)
+
+    def settimeout(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'settimeout', *args, **kwargs)
+
+    def gettimeout(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'gettimeout', *args, **kwargs)
+
+    def setsockopt(self, *args, **kwargs):
+        return object.__getattribute__(self, r'_observedcall')(
+            r'setsockopt', *args, **kwargs)
+
 DATA_ESCAPE_MAP = {pycompat.bytechr(i): br'\x%02x' % i for i in range(256)}
 DATA_ESCAPE_MAP.update({
     b'\\': b'\\\\',
@@ -709,15 +823,7 @@ 
 
     return DATA_ESCAPE_RE.sub(lambda m: DATA_ESCAPE_MAP[m.group(0)], s)
 
-class fileobjectobserver(object):
-    """Logs file object activity."""
-    def __init__(self, fh, name, reads=True, writes=True, logdata=False):
-        self.fh = fh
-        self.name = name
-        self.logdata = logdata
-        self.reads = reads
-        self.writes = writes
-
+class baseproxyobserver(object):
     def _writedata(self, data):
         if not self.logdata:
             self.fh.write('\n')
@@ -734,6 +840,15 @@ 
         for line in lines:
             self.fh.write('%s>     %s\n' % (self.name, escapedata(line)))
 
+class fileobjectobserver(baseproxyobserver):
+    """Logs file object activity."""
+    def __init__(self, fh, name, reads=True, writes=True, logdata=False):
+        self.fh = fh
+        self.name = name
+        self.logdata = logdata
+        self.reads = reads
+        self.writes = writes
+
     def read(self, res, size=-1):
         if not self.reads:
             return
@@ -796,6 +911,119 @@ 
                                   logdata=logdata)
     return fileobjectproxy(fh, observer)
 
+class socketobserver(baseproxyobserver):
+    """Logs socket activity."""
+    def __init__(self, fh, name, reads=True, writes=True, states=True,
+                 logdata=False):
+        self.fh = fh
+        self.name = name
+        self.reads = reads
+        self.writes = writes
+        self.states = states
+        self.logdata = logdata
+
+    def makefile(self, res, mode=None, bufsize=None):
+        if not self.states:
+            return
+
+        self.fh.write('%s> makefile(%r, %r)\n' % (
+            self.name, mode, bufsize))
+
+    def recv(self, res, size, flags=0):
+        if not self.reads:
+            return
+
+        self.fh.write('%s> recv(%d, %d) -> %d' % (
+            self.name, size, flags, len(res)))
+        self._writedata(res)
+
+    def recvfrom(self, res, size, flags=0):
+        if not self.reads:
+            return
+
+        self.fh.write('%s> recvfrom(%d, %d) -> %d' % (
+            self.name, size, flags, len(res[0])))
+        self._writedata(res[0])
+
+    def recvfrom_into(self, res, buf, size, flags=0):
+        if not self.reads:
+            return
+
+        self.fh.write('%s> recvfrom_into(%d, %d) -> %d' % (
+            self.name, size, flags, res[0]))
+        self._writedata(buf[0:res[0]])
+
+    def recv_into(self, res, buf, size=0, flags=0):
+        if not self.reads:
+            return
+
+        self.fh.write('%s> recv_into(%d, %d) -> %d' % (
+            self.name, size, flags, res))
+        self._writedata(buf[0:res])
+
+    def send(self, res, data, flags=0):
+        if not self.writes:
+            return
+
+        self.fh.write('%s> send(%d, %d) -> %d' % (
+            self.name, len(data), flags, len(res)))
+        self._writedata(data)
+
+    def sendall(self, res, data, flags=0):
+        if not self.writes:
+            return
+
+        # Returns None on success. So don't bother reporting return value.
+        self.fh.write('%s> sendall(%d, %d)' % (
+            self.name, len(data), flags))
+        self._writedata(data)
+
+    def sendto(self, res, data, flagsoraddress, address=None):
+        if not self.writes:
+            return
+
+        if address:
+            flags = flagsoraddress
+        else:
+            flags = 0
+
+        self.fh.write('%s> sendto(%d, %d, %r) -> %d' % (
+            self.name, len(data), flags, address, res))
+        self._writedata(data)
+
+    def setblocking(self, res, flag):
+        if not self.states:
+            return
+
+        self.fh.write('%s> setblocking(%r)\n' % (self.name, flag))
+
+    def settimeout(self, res, value):
+        if not self.states:
+            return
+
+        self.fh.write('%s> settimeout(%r)\n' % (self.name, value))
+
+    def gettimeout(self, res):
+        if not self.states:
+            return
+
+        self.fh.write('%s> gettimeout() -> %f\n' % (self.name, res))
+
+    def setsockopt(self, level, optname, value):
+        if not self.states:
+            return
+
+        self.fh.write('%s> setsockopt(%r, %r, %r) -> %r\n' % (
+            self.name, level, optname, value))
+
+def makeloggingsocket(logh, fh, name, reads=True, writes=True, states=True,
+                      logdata=False):
+    """Turn a socket into a logging socket."""
+
+    observer = socketobserver(logh, name, reads=reads, writes=writes,
+                              states=states, logdata=logdata)
+    return socketproxy(fh, observer)
+
 def version():
     """Return version information if available."""
     try: