Opened 5 years ago

Closed 5 years ago

#7674 closed enhancement (implemented)

add version requirements to Event classes

Reported by: robinson Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords: events
Cc: seankrobinson@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This first submission is a Request For Comments, I still need to write tests before I ask to add this branch.

While reviewing SignalEvent, I realized it would not work for the tor version I am running.

ProtocolError: SETEVENTS received unexpected response
Unrecognized event "SIGNAL"

I thought the error message could be more meaningful (e.g.

ProtocolError: SETEVENTS received unexpected response
SIGNAL event requires Tor version 0.2.3.1-alpha or later

So, this patch series adds a version check to Controller.add_event_listener() and provides versions to Event classes (with a default version in Event itself).

Is this something we should add to Stem? Is this the format to use? I'd appreciate any feedback.

Pull the exp-event-versions-v1 branch from git://gitorious.org/stem-robinson/stem-robinson.git or view commit logs @ https://gitorious.org/stem-robinson/stem-robinson/commits/exp-event-versions-v1

Child Tickets

Change History (4)

comment:1 Changed 5 years ago by robinson

  • Cc seankrobinson@… added
  • Keywords events added

comment:2 Changed 5 years ago by atagar

Shame on me. I saw your first ticket, looked over your other branches, and loved exp-event-versions-v1 so I pushed. I should have read the other tickets first...

Is this something we should add to Stem? Is this the format to use? I'd appreciate any feedback.

Yup, it looks like a great addition. I squashed the commits a bit and made a minor tweak so add_event_listener() would be all-or-nothing...

https://gitweb.torproject.org/stem.git/commitdiff/40827b365e3a5f3dac6d1f8e8ffddd589be6cd70

But otherwise I can't think of any suggestions. Leaving this open since indeed a test or two would be good to have.

comment:3 Changed 5 years ago by robinson

No worries. I was not sure the event version feature would be desired and I did not want to demand its addition to Stem. But, if you like it: great!

You're right, the all-or-nothing approach is better. The single listener_lock over the entire event activation is also correct, IMHO.

The tests in the following branch require the fixed mock_method in ticket #7686. The exp-event-versions-v2 branch contains just changes after your push to master of the previous event version changes.

Pull the exp-event-versions-v2 branch from git://gitorious.org/stem-robinson/stem-robinson.git or view commit logs @ https://gitorious.org/stem-robinson/stem-robinson/commits/exp-event-versions-v2

comment:4 Changed 5 years ago by atagar

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