Errback when bootstrapping the `TorConfig`

[felipedau]

So, we are planning to release a new unMessage version and recently I have been working on a few things to prevent/handle errors that might frustrate users. When we configure the control port filter, everything works fine, but I forgot to handle when a filter prevents us from acquiring the process so users get a more friendly message.

It looks like the failure occurs in torcontrolprotocol.py at:

811         eventnames = yield self.get_info('events/names')   

because that command is filtered and log.err at the end prevents this error from being handled. I do not fully understand the bootstrapping "magic" and I am not sure if this fix is actually a fix, but makes sense to me.

Is it correct?

P.S. I intend to handle an error with a 510 code. I am not too familiar to the control spec, but do you think it would be useful (and is possible) to make classes for these too like we did for the SOCKS ones?

Thanks!

[coveralls]

Coverage remained the same at 99.945% when pulling 6bc808b7327382af9452a14370e0c4553b3c1489 on felipedau:fix/errback-config-bootstrap into cb670f46f6f85b644c5d60ab63eee04fcee61d09 on meejah:master.

[codecov-io]

Codecov Report

Merging #236 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #236   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          20       20           
  Lines        3657     3657           
=======================================
  Hits         3655     3655           
  Misses          2        2

Impacted Files	Coverage Δ
txtorcon/torconfig.py	100% <ø> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cb670f4...6bc808b. Read the comment docs.

[meejah]

I think this fix is great, yes. Thanks!

As for the errors, you get a TorProtocolError on any 500-level responses, and it has a .code. Are you talking about separate subclasses of this for each "500" level error that tor happens to emit? My inclination would be to not do this.

For your use-case specifically, the error-code isn't actually coming from Tor at all, but from the filter -- and I really wouldn't want to "burn in" whatever random number different filters etc choose for errors. Are you planning for unmessage to do something different depending on the code? (i.e. what will you do besides show the message to the user and fail, no matter if it's "512" versus "599")

[felipedau]

I think this fix is great, yes. Thanks!

Cool! Do you think this breaks things? I think that probably does, but just the ones that were "too optimistic" and did not have any code expecting an errback.

As for the errors, you get a TorProtocolError on any 500-level responses, and it has a .code. Are you talking about separate subclasses of this for each "500" level error that tor happens to emit? My inclination would be to not do this.

Ahhh right, 510 was set by the filter! I was suggesting to have subclasses for each of the errors that are in the spec and might be presented to anything that calls txtorcon. It's just a way of people checking errors with classes instead of harcoded numbers, but I am not sure if anyone does that.

For your use-case specifically, the error-code isn't actually coming from Tor at all, but from the filter -- and I really wouldn't want to "burn in" whatever random number different filters etc choose for errors. Are you planning for unmessage to do something different depending on the code? (i.e. what will you do besides show the message to the user and fail, no matter if it's "512" versus "599")

That makes a lot of sense. I was going to if e.code == 510 tell they have a control port filter running and need to set up a profile (and maybe a link to the docs or something like that) and just display a regular error message otherwise. Maybe I can't even do that. I might have to look at the message, right?

I just checked and 510 is actually "Unrecognized command" in the spec, but the filter displays a different message. I might be wrong, but I remember in one of the discussions we had with @adrelanos that it should not be visible for anything that was trying to use the process that there is a filter in the middle - if you can't use that you just can't. Should the filter not even change the message of 510? Or as it is already using a different message, use a different code too?

(Am I overcomplicating things?)

[meejah]

IMO, you probably don't need to (and shouldn't care) what error-number it is (beyond "it's in the 500 range") -- just tell the user something bad happened and show them the user. It seems better to trigger the hints to the user about control-port filters on something more reliable -- e.g. checking if you're running on Whonix etc.

If you can do different things based on the actual code, I'm still not necessarily sold on "different subclasses" -- seems more complicated to isinstance(e, bunch_of_subclasses) rather than just checking the code. I also don't like having to add/remove/change actual classes in txtorcon if a new error-code pops up or goes away in Tor (and then you're kind of tied to tor-versions etc too). Still, it would mirror the socks stuff so .... :man_shrugging:

[meejah]

oh, re: breaking things. Maybe. But "just log and ignore the error" is definitely not great (or correct) and would result in a broken thing anyway.

[felipedau]

IMO, you probably don't need to (and shouldn't care) what error-number it is (beyond "it's in the 500 range") -- just tell the user something bad happened and show them the user. It seems better to trigger the hints to the user about control-port filters on something more reliable -- e.g. checking if you're running on Whonix etc.

If you can do different things based on the actual code, I'm still not necessarily sold on "different subclasses" -- seems more complicated to isinstance(e, bunch_of_subclasses) rather than just checking the code. I also don't like having to add/remove/change actual classes in txtorcon if a new error-code pops up or goes away in Tor (and then you're kind of tied to tor-versions etc too). Still, it would mirror the socks stuff so .... :man_shrugging:

Ahh , I see. Thanks for the explanation, makes sense :)

oh, re: breaking things. Maybe. But "just log and ignore the error" is definitely not great (or correct) and would result in a broken thing anyway.

Right, an error in a place like that makes it impossible to use the process.

Thanks!