Autodetect response type in ControlMessage.from_str()

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.

added component::archived/stem controller easy owner::atagar priority::medium resolution::implemented status::closed type::enhancement labels

Trac:
Keywords: N/A deleted, controller easy added

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

Trac:
Username: sreenatha

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

Trac:
Status: new to closed
Resolution: N/A to wontfix

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").

Trac:
Resolution: wontfix to N/A
Status: closed to reopened

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?).

Yes, msg_type sounds good.

Argument added...

https://gitweb.torproject.org/stem.git/commitdiff/0efbdf80f94574805b0fa080965bb509d535a4db

Trac:
Status: reopened to closed
Resolution: N/A to implemented

closed