From 128d2db073cbc6eaba1a186f0b67a233c22ead94 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 Aug 2023 16:22:43 -0600 Subject: [PATCH 1/8] adjust upstream defaults --- txtorcon/torcontrolprotocol.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/txtorcon/torcontrolprotocol.py b/txtorcon/torcontrolprotocol.py index 887bd0ae..ab38c40f 100644 --- a/txtorcon/torcontrolprotocol.py +++ b/txtorcon/torcontrolprotocol.py @@ -232,6 +232,10 @@ class TorControlProtocol(LineOnlyReceiver): :class:`txtorcon.TorState`, which is also the place to go if you wish to add your own stream or circuit listeners. """ + # override Twisted's LineOnlyReceiver maximum line-length. At + # least "GETINFO md/id/X" for some Xse exceeds 16384 (2**14, the + # default) and thus causes the control connection to fail. + MAX_LENGTH = 2 ** 20 def __init__(self, password_function=None): """ From 1095879b8a6cdd5ef6325eabbf6e09e4d9800a6c Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 Aug 2023 16:22:50 -0600 Subject: [PATCH 2/8] redundant --- txtorcon/torcontrolprotocol.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/txtorcon/torcontrolprotocol.py b/txtorcon/torcontrolprotocol.py index ab38c40f..0de29276 100644 --- a/txtorcon/torcontrolprotocol.py +++ b/txtorcon/torcontrolprotocol.py @@ -278,11 +278,6 @@ def __init__(self, password_function=None): :func:`when_disconnected` instead) """ - self._when_disconnected = SingleObserver() - """ - Internal use. A :class:`SingleObserver` for when_disconnected() - """ - self._when_disconnected = SingleObserver() """ Private. See :func:`.when_disconnected` From d9ebf0c240579a2e1be280a492cae1ae76060514 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 Aug 2023 16:24:20 -0600 Subject: [PATCH 3/8] consistently use error if we're disconnected --- txtorcon/torcontrolprotocol.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/txtorcon/torcontrolprotocol.py b/txtorcon/torcontrolprotocol.py index 0de29276..59593039 100644 --- a/txtorcon/torcontrolprotocol.py +++ b/txtorcon/torcontrolprotocol.py @@ -691,10 +691,14 @@ def connectionMade(self): def connectionLost(self, reason): "Protocol API" txtorlog.msg('connection terminated: ' + str(reason)) - if reason.check(ConnectionDone): - self._when_disconnected.fire(self) - else: - self._when_disconnected.fire(reason) + self._when_disconnected.fire( + Failure( + TorDisconnectError( + text="Tor connection terminated", + error=reason, + ) + ) + ) # ...and this is why we don't do on_disconnect = Deferred() :( # and instead should have had on_disconnect() method that @@ -711,8 +715,10 @@ def connectionLost(self, reason): else: self.on_disconnect.errback(reason) self.on_disconnect = None - self._when_disconnected.fire(self) + outstanding = [self.command] + self.commands if self.command else self.commands + self.command = None + self.defer = None for d, cmd, cmd_arg in outstanding: if not d.called: d.errback( From cca6d69a1e794a1b35c2fd12d127836e43d32a7d Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 Aug 2023 17:33:28 -0600 Subject: [PATCH 4/8] if we already disconnected, all commands fail --- test/test_torcontrolprotocol.py | 2 +- txtorcon/torcontrolprotocol.py | 4 ++++ txtorcon/util.py | 13 +++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/test/test_torcontrolprotocol.py b/test/test_torcontrolprotocol.py index 23ddeece..19e55d8c 100644 --- a/test/test_torcontrolprotocol.py +++ b/test/test_torcontrolprotocol.py @@ -226,7 +226,7 @@ def it_was_called(arg): it_was_called.yes = False d = self.protocol.when_disconnected() - d.addCallback(it_was_called) + d.addBoth(it_was_called) f = failure.Failure(error.ConnectionDone("It's all over")) self.protocol.connectionLost(f) self.assertTrue(it_was_called.yes) diff --git a/txtorcon/torcontrolprotocol.py b/txtorcon/torcontrolprotocol.py index 59593039..34805279 100644 --- a/txtorcon/torcontrolprotocol.py +++ b/txtorcon/torcontrolprotocol.py @@ -759,6 +759,10 @@ def _maybe_issue_command(self): if len(self.commands): self.command = self.commands.pop(0) (d, cmd, cmd_arg) = self.command + + if self._when_disconnected.already_fired(d): + return + self.defer = d self.debuglog.write(cmd + b'\n') diff --git a/txtorcon/util.py b/txtorcon/util.py index 4b772e32..406a0f59 100644 --- a/txtorcon/util.py +++ b/txtorcon/util.py @@ -473,6 +473,19 @@ def __init__(self): self._observers = [] self._fired = self._NotFired + def has_fired(self): + return self._fired is not self._NotFired + + def already_fired(self, d): + """ + If we have already fired, callback `d` with our result. + :returns bool: True if we already fired, False otherwise + """ + if self.has_fired(): + d.callback(self._fired) + return True + return False + def when_fired(self): d = defer.Deferred() if self._fired is not self._NotFired: From 66681ea576b208bd8e6e2eb4a84d74d238fd0cd3 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 Aug 2023 17:33:51 -0600 Subject: [PATCH 5/8] log is binary --- txtorcon/torcontrolprotocol.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/txtorcon/torcontrolprotocol.py b/txtorcon/torcontrolprotocol.py index 34805279..ec5295eb 100644 --- a/txtorcon/torcontrolprotocol.py +++ b/txtorcon/torcontrolprotocol.py @@ -355,7 +355,7 @@ def setup(proto): self.stop_debug() def start_debug(self): - self.debuglog = open('txtorcon-debug.log', 'w') + self.debuglog = open('txtorcon-debug.log', 'wb') def stop_debug(self): def noop(*args, **kw): From 7082ca0261029ac9542f79584dd93b524ce51b54 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 Aug 2023 17:37:19 -0600 Subject: [PATCH 6/8] test subsequent commands after disconnect --- test/test_torcontrolprotocol.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/test_torcontrolprotocol.py b/test/test_torcontrolprotocol.py index 19e55d8c..e15bdf03 100644 --- a/test/test_torcontrolprotocol.py +++ b/test/test_torcontrolprotocol.py @@ -284,6 +284,31 @@ def it_was_called(f): self.protocol.connectionLost(f) self.assertEqual(it_was_called.count, 2) + def test_disconnect_subsequent_commands(self): + """ + commands issued after disconnect should errback + """ + + def it_was_called(f): + str(f) + it_was_called.count += 1 + return None + it_was_called.count = 0 + + # one outstanding command + d0 = self.protocol.queue_command("some command0") + d0.addErrback(it_was_called) + self.protocol.on_disconnect.addErrback(lambda _: None) + + f = failure.Failure(RuntimeError("The thing didn't do the stuff.")) + self.protocol.connectionLost(f) + + # one command issued _after_ we've disconnected + d1 = self.protocol.queue_command("some command1") + d1.addErrback(it_was_called) + + self.assertEqual(it_was_called.count, 2) + class ProtocolTests(unittest.TestCase): From 3e8493c2c3cacf65e0c65436ee25f70404fe270a Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 Aug 2023 17:39:05 -0600 Subject: [PATCH 7/8] news --- docs/releases.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/releases.rst b/docs/releases.rst index ea326189..517a41c0 100644 --- a/docs/releases.rst +++ b/docs/releases.rst @@ -18,6 +18,7 @@ See also :ref:`api_stability`. `git main `_ *will likely become v23.6.0* * Fix test-failures on Python 3.12 + * Particular GETINFO hanging (`#389 `_) v23.5.0 From caf0b389e16e63dd06992f0a2234bbca6ed5ca75 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 29 Aug 2023 17:58:26 -0600 Subject: [PATCH 8/8] more justification --- txtorcon/torcontrolprotocol.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/txtorcon/torcontrolprotocol.py b/txtorcon/torcontrolprotocol.py index ec5295eb..e882160a 100644 --- a/txtorcon/torcontrolprotocol.py +++ b/txtorcon/torcontrolprotocol.py @@ -234,7 +234,9 @@ class TorControlProtocol(LineOnlyReceiver): """ # override Twisted's LineOnlyReceiver maximum line-length. At # least "GETINFO md/id/X" for some Xse exceeds 16384 (2**14, the - # default) and thus causes the control connection to fail. + # default) and thus causes the control connection to + # fail. control.c defines MAX_COMMAND_LINE_LENGTH as 1024*1024 so + # we use that MAX_LENGTH = 2 ** 20 def __init__(self, password_function=None):