Opened 6 years ago

Closed 5 years ago

#4257 closed enhancement (wontfix)

Stem / TorCtl Integration

Reported by: atagar Owned by: mikeperry
Priority: Medium Milestone:
Component: Torctl Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

As the saying goes: commit early, commit often. Rather than completing stem then tossing it over the wall, the stem-integration branch of my TorCtl repo [1] has a bite sized commit to start making use of it:
https://gitweb.torproject.org/atagar/pytorctl.git/commitdiff/d3a12d5724e44dac9096a56d0706bbc342efa304

This replaces TorCtl's _read_reply and related util functionality with stem's ControlMessage class and parsing [2], which has unit [3] and integration [4] tests. Doing a series of small commits like this will let TorCtl users take advantage of the tests I'm writing for stem without a need for changes on their part.

Cheers! -Damian

PS. Historically my TorCtl patches collect a lot of dust, so I'm testing the waters a bit with this ticket. If there isn't time or interest on your part to make this happen then it ain't gonna happen and I'll focus on stem solely rather than using it as a means to improve TorCtl.

[1] https://gitweb.torproject.org/atagar/pytorctl.git/shortlog/refs/heads/stem-integration
[2] https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/types.py
[3] https://gitweb.torproject.org/stem.git/blob/HEAD:/test/unit/message.py
[4] https://gitweb.torproject.org/stem.git/blob/HEAD:/test/integ/message.py

Child Tickets

Change History (9)

comment:1 Changed 6 years ago by atagar

  • Status changed from new to needs_review

comment:2 Changed 6 years ago by atagar

  • Owner set to mikeperry
  • Status changed from needs_review to assigned

comment:3 Changed 6 years ago by atagar

  • Status changed from assigned to needs_review

... wow trac's status changing sucks. Ok, to make this both owned by mike and flagged as needing review guess I need to to a status change again.

comment:4 Changed 5 years ago by mikeperry

What should I do with this? Is it ready to be tested for TorCtl compatibility, or should I just be reviewing it for API sanity?

comment:5 Changed 5 years ago by atagar

What should I do with this?

That depends on how we want TorCtl and stem to be related. As I see it there's a couple options...

  • Option 1: Incrementally replace TorCtl's implementation with calls to stem. That is what this change is for.

advantages:

  • the migration will be transparent to TorCtl users
  • TorCtl will begin to have testing for its core functionality
  • changes are gradual so if there's any compatibility issues then they will be easier to catch

disadvantages:

  • this requires active involvement by both me and the TorCtl maintainer
  • we don't have any testing for TorCtl or its users (though I'd still argue that a series of small changes will be less painful than a single huge one)
  • if this collects dust for months then it'll become unreasonable to continue with this approach
  • Option 2: Big bang migrations later. In this case I'd ignore TorCtl for now, finish stem, then either help to rewrite TorCtl users or make a TorCtl facade. I'm not sure if it's an advantage or disadvantage, but stem would be a proper fork of TorCtl so users could opt to use either.

advantages:

  • no up front involvement by either of us
  • narrows the window where dependencies change
  • for better or worse TorCtl's codebase never changes

disadvantages:

  • TorCtl doesn't get any benefit from stem
  • big bang migrations are more painful for users
  • compatibility issues are harder to track down

or should I just be reviewing it for API sanity?

I'll let you know when it's ready for a review. At present just the socket listening and message parsing components are complete. At this point the user facing functionality would be easy to write, but my focus is on shoring up the utilities and testing first.

Cheers! -Damian

comment:6 Changed 5 years ago by mikeperry

Well, I like the sound of option 1's incremental approach.. But the problem with option 1 is that it creates work for me right now, and possibly continuously in terms of needing to test and keep testing some or all of our infrastructure on your code.

Unless I can convince you it would be fun to run an exit scanner using stem underneath? Or perhaps make a simple IRC bot front end for SoaT that uses stem underneath and can execute single-target tests (--exit and --target) so we can quickly spot-test people's claims right in #tor-dev?

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

Unless I can convince you it would be fun to run an exit scanner using stem underneath?

I have my hands full enough getting this library written, and the bar a change needs to pass before being deemed sufficient safe seems like a call that you should be making. I manually test TorCtl changes before sending them on to you (of course - details in commit description) so from my point of view they're good to go.

That said, an alarming infrastructure or hacking on soat are among the projects I'm thinking about for after stem.

comment:8 in reply to: ↑ 7 Changed 5 years ago by aagbsn

Replying to atagar:

Unless I can convince you it would be fun to run an exit scanner using stem underneath?

I have my hands full enough getting this library written, and the bar a change needs to pass before being deemed sufficient safe seems like a call that you should be making. I manually test TorCtl changes before sending them on to you (of course - details in commit description) so from my point of view they're good to go.

From my perspective, the first test I run against TorCtl is BwAuthority. If that test fails, I am willing to investigate, but I have my hands full enough too :-). See for instance #4015. This issue is a show-stopper for BwAuthority; an issue I have put several days of time into.

If we want to proceed with Stem & TorCtl integration, core applications have to work. It would be a huge help to mike and I if you could test your TorCtl changes against live BwAuthority and ExitAuthority instances. If you aren't sure where to start, I can help you set these up and answer any questions. And if you don't have infrastructure, I can give you access to infrastructure.

--Aaron

comment:9 Changed 5 years ago by atagar

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

It would be a huge help to mike and I if you could test your TorCtl changes against live BwAuthority and ExitAuthority instances.

No.

I'm happy to help people develop against stem, and would even go as far as to do all of the stem/TorCtl integration work and stand alone testing. However, if that in addition to operating and testing your service is the requirement to move forward with this then I'd rather go with option #2. Both services are black boxes to me and I'm not interested in learning to differentiate the normal issues they're having verses an issue that may be caused by stem.

It sounds like there's no bandwidth, even if I do most of the work, to make this happen so closing this ticket.

Note: See TracTickets for help on using tickets.