Opened 4 years ago

Closed 4 years ago

#8605 closed enhancement (implemented)

Autodetect response type in ControlMessage.from_str()

Reported by: atagar Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords: controller easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Presently the from_str() method provides back a ControlMessage instance, which the caller then needs to call convert() on to translate it to the appropriate subclass. This is pretty clunky and, when responses have prefix headers (like '650 BUILDTIMEOUT_SET') we should be able to detect the type on their behalf.

We should check if convert() is idempotent (ie, calling it twice on a control message is safe). If so then we can change the default from_str() behavior, but if not we'll need to do something different to preserve backward compatibility.

Child Tickets

Change History (7)

comment:1 Changed 4 years ago by atagar

  • Keywords controller easy added

comment:2 Changed 4 years ago by sreenatha

Modified from_str to identify responses of types PROTOCOLINFO, AUTHCHALLENGE  and EVENT. Unable to find a solid way of identifying other response types(GETINFO, GETCONF, MAPADDRESS and SINGLELINE). Patch for response/__init__.py available at  https://github.com/lucyd/stem/commit/03399741433551dabbfb9060c3d736f54033b839

comment:3 Changed 4 years ago by atagar

  • Resolution set to wontfix
  • Status changed from new to closed

Hi sreenatha, thanks for looking into this!

Unable to find a solid way of identifying other response types(GETINFO, GETCONF, MAPADDRESS and SINGLELINE).

Hmmm, on reflection you're right. Most controller responses won't have a type associated with them...

atagar@morrigan:~$ telnet localhost 9051
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
AUTHENTICATE
250 OK

GETINFO version
250-version=0.2.4.10-alpha-dev (git-8be6058d8f31e578)
250 OK

I'd rather not have from_str() automatically convert a small handful of message types but fail for highly common ones like GETINFO and GETCONF. Guess we will need to leave the conversion to our callers after all... oh well, thanks for investigating it. :)

Couple nitpicks for the future: thank you for including the ticket url in your commit message, but please also use a more descriptive commit message (take a look at other commits in our repository for examples). Also, the control-spec says that an event's response code is '650'. Simply checking that the code starts with a '6' probably wouldn't have been a good idea...

https://gitweb.torproject.org/torspec.git/blob/HEAD:/control-spec.txt#l1216

Cheers! -Damian

comment:4 Changed 4 years ago by meejah

  • Resolution wontfix deleted
  • Status changed from closed to reopened

For the "synchronous" (non-650) responses, you will have exactly one outstanding request -- could this be provided as a "hint" (defaulting to None)? Then for any 650 messages, you can autodetect, and for anything else if the hint is None it's an error (or not convert()-ed?) and otherwise the hint is used to determine the correct type?

Then, callers simply provide the outstanding message as the hint (if they don't have one, they use None and it'll "just work").

comment:5 Changed 4 years ago by atagar

For the "synchronous" (non-650) responses, you will have exactly one outstanding request

Yes, which is what the Controller does. It issues a GETINFO request then knows that the reply it gets should be parsed as such. The trouble here is that when txtorcon calls from_str() we *only* get the reply so we're missing that context.

If you'd like then we can combine from_str() and convert() by adding an argument to from_str() for the type. That is to say, change from_str to...

def from_str(content, msg_type = None, **kwargs):
  msg = stem.socket.recv_message(StringIO.StringIO(content))

  if msg_type is not None:
    convert(msg_type, msg, **kwargs)

  return msg

Would this work for you? I'd rather not have stem attempt to infer the msg_type when undefined since that would lead to pretty confusing behaviour (for instance, why are PROTOCOLINFO responses automatically converted but not GETINFO?).

comment:6 Changed 4 years ago by meejah

Yes, msg_type sounds good.

comment:7 Changed 4 years ago by atagar

  • Resolution set to implemented
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.