Opened 4 years ago

Closed 4 years ago

#7328 closed enhancement (implemented)

Event Handling

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

Description

Hi Ravi. Pushed a start for tor event handling to the 'events' branch of my repository...

https://gitweb.torproject.org/user/atagar/stem.git/shortlog/refs/heads/events

Usage is pretty simple. The following for instance would print the bytes sent and received by tor for five seconds...

import time
from stem.control import Controller, EventType

def print_bw(event):
  print "sent: %i, received: %i" % (event.written, event.read)

with Controller.from_port(control_port = 9051) as controller:
  controller.authenticate()
  controller.add_event_listener(print_bw, EventType.BW)
  time.sleep(5)

Cheers! -Damian

Child Tickets

TicketTypeStatusOwnerSummary
#7597enhancementclosedatagarSend arbitrary parameters to functors
#7616enhancementclosedatagaradd STREAM_BW event handler

Change History (12)

comment:1 Changed 4 years ago by atagar

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by atagar

  • Keywords controller added

comment:3 Changed 4 years ago by robinson

  • Status changed from needs_review to needs_revision

atagar,

So, I went through the events branch commit-by-commit and overall it looks very good. Three quick comments and I'll try to open sub-tickets for two other items.

I like that CIRC Event returns a fingerprint and nickname tuple in path. This was a minor bug in your personal TorCtl tree event handler that I just recently (this week) fixed for myself.

Is it possible that stem.StreamClosureReason values map to an interger starting with 1 (i.e. stem.StreamClosureReason.index_of(stem.StreamClosureReason.MISC) = 1). If so, I have written a Controller.close_stream method which could re-use stem.StreamClosureReason values for RELAY_END reason.

The commit titled "Revised behavior for document signature methods" is not relevant to events. It's not a big deal, but to avoid side effects merging this branch, maybe you could cherry-pick around it?

comment:4 Changed 4 years ago by robinson

After my last comment, I noticed that there are two new commits on your events branch. To clarify my earlier examination of commits, I only reviewed through "Parsing AUTHDIR_NEWDESCS events" (commit 5b59e31ce1bcf23b).

comment:5 follow-up: Changed 4 years ago by atagar

  • Status changed from needs_revision to assigned

Thanks, robinson, for the code review!

Is it possible that stem.StreamClosureReason values map to an interger starting with 1...

Not unless we add a filler value at the start of the enum. I'd advise against using stem.StreamClosureReason indices, and instead use a dict that maps ints to stem.StreamClosureReason. The reason for this is less the offset that you mentioned, and more because StreamClosureReason includes 'END' and 'PRIVATE_ADDR'. Those are values allowed for in STREAM events, but not part of RELAY_END.

The commit titled "Revised behavior for document signature methods" is not relevant to events. It's not a big deal, but to avoid side effects merging this branch, maybe you could cherry-pick around it?

Good point. Cherry-picked fe16cce, pushed it to master, and rewrote my events branch to exclude it.

I'm swapping this back to the assigned status since I'm less than half way through the event types. I'll swap this back to 'needs review' when I'm done, though more code reviews as I go along would be very welcome.

comment:6 in reply to: ↑ 5 Changed 4 years ago by robinson

Replying to atagar:

Is it possible that stem.StreamClosureReason values map to an interger starting with 1...

[No]...because StreamClosureReason includes 'END' and 'PRIVATE_ADDR'. Those are values allowed for in STREAM events, but not part of RELAY_END.

On further reflection, I agree that StreamClosureReason should not be re-used for RELAY_END. I guess I'll rework my close_stream patch before submitting it. 8-)

I'm swapping this back to the assigned status since I'm less than half way through the event types. I'll swap this back to 'needs review' when I'm done, though more code reviews as I go along would be very welcome.

Please push the current event handling framework to the main Stem repository. Anyone else (read: me) wanting to develop event handler classes may find it easier to work against that repo. Any event handler classes you add will just be enhancements to the base event code.

comment:7 Changed 4 years ago by atagar

On further reflection, I agree that StreamClosureReason should not be re-used for RELAY_END. I guess I'll rework my close_stream patch before submitting it. 8-)

Actually, after thinking about it maybe we could. StreamClosureReason is a superset of RELAY_END so maybe add a stem.RelayEnd enum and use it in the StreamClosureReason constructor? The close_stream() method could pretty easily account for the index offset.

Please push the current event handling framework to the main Stem repository.

Ok, will do later today. I was gonna wait until the feature branch was complete but if folks want to develop against it then I agree that it should be merged early.

comment:8 Changed 4 years ago by atagar

Implemented a few more event types (GUARD, NS, CLIENTS_SEEN, and NEWCONSENSUS) and pushed...

https://gitweb.torproject.org/stem.git/commitdiff/42872dd08e81d6b341654ab85969df2ed77a7397?hp=e1772bb9ad9ad2f6f085e2c0f489214d2f7fd6ee

Keeping this ticket open to track work on the remaining event types...

comment:11 Changed 4 years ago by atagar

Pushed support for CONF_CHANGED and CIRC_MINOR events...

https://gitweb.torproject.org/stem.git/commitdiff/bb458df4ac47c1405ce7e7f7e3e15ececf485b4b
https://gitweb.torproject.org/stem.git/commitdiff/3ba521dfa2f2a6f023e7b92fa24bcfb910f1fe78

That's all of the event types except STREAM_BW which is being tracked in another ticket. I'd like to resolve this one but trac won't let me do so while child tickets are active so this is pending those.

Cheers! -Damian

comment:12 Changed 4 years ago by atagar

  • Resolution set to implemented
  • Status changed from assigned to closed

Children ticket are resolved so resolving this as well. Over the next couple days I'll be cleaning up the implementation a bit (there's quite a bit of redundant code), but there's probably no need to keep this ticket open for it.

Note: See TracTickets for help on using tickets.