Opened 5 years ago

Closed 5 years ago

#7152 closed task (implemented)

Implement Controller.attach_stream

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

Description

A wrapper around the ATTACHSTREAM control command. My implementation is here.

The test is a little wonky right now. We should put off reviewing/merging it until Stem has event handling, and until the test is fixed to use it.

Child Tickets

Change History (14)

comment:1 Changed 5 years ago by atagar

Status: newneeds_revision

Hi Ravi. Glad you're back! I'll resume sending you code reviews, I should have something soonish (didn't make sense to continue interrupting you while you were on your trip - hope it was fun).

Change looks good...

+ response = self.msg("ATTACHSTREAM %s %s%s" % (str(stream), str(circuit), hop_str))

The str() calls aren't necessary...

>>> "number %s" % 5
'number 5'

It doesn't make a difference though for clarity I'd probably use %i...

>>> "number %i" % 5
'number 5'

+ time.sleep(5) # wait for the circuit to be built.
+ # TODO: This is fragile. It doesn't check if the circuit was successfully
+ # created. We should use the CIRC events once it exists

Sleep should be avoided in tests if at all possible. Somewhat hypocritically here's an example of listening for raw events in tests that have a sleep...

https://gitweb.torproject.org/stem.git/blob/HEAD:/test/integ/control/base_controller.py#l129

+ circs = [circuit.split(" ") for circuit in controller.get_info("circuit-status").split("\n")]
+ exit_circ = filter(lambda circuit: circuit[0] is circuit_id, circs)[0]
+ exit_relay_id = exit_circ[2].split(",")[-1][1:41]

We should probably add a nicer method for this, like arm's torTools.getCircuits...

https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torTools.py#l1051

+ # Get our exit relay's IP address from the descriptor
+ exit_ip = [line for line in controller.get_info("ns/id/" +
+ exit_relay_id).split("\n") if line.startswith("r ")][0].split(" ")[-3]

Unnecessarily icky, please use get_network_status()...

https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/control.py#l701

+ # Helper function to get the result of a threaded function call
+ def thread_helper(func, result, *args, kwargs):
+ result[0] = func(*args,
kwargs)

...

Hmmm, there must be a nicer way of doing this but I'm not sure what it is at the moment. I'm presently working on stem's tutorials and in one of those I want to exemplify using stem for client traffic so I'll try to look into making this more elegant.

comment:2 Changed 5 years ago by atagar

Keywords: controller added

comment:3 Changed 5 years ago by robinson

Status: needs_revisionneeds_information

Ravi,

There's some extraneous stuff (e.g. partial Circuit class, incomplete get_circuits method) in the latest commit at the link you provided. I was not able to review your code without doing non-trivial clean-ups, first. So I stopped. Do you have a patch/commit series which focuses just on attach_stream and its tests?

comment:4 Changed 5 years ago by neena

Damian, Robinson

I'm not sure what I'm doing wrong. But, I'm unable to handle STREAM events. Would appreciate help, I've been stuck with this for a while now... code here. I'm adding a pdb.set_trace() in handle_streamcreated to check if it's being run.

I also wrote a get_circuits() method, should be useful to make the other circuit related tests neater (close_circuit etc). Though, I find it a little convoluted to first make it an event and then parse it.

comment:5 Changed 5 years ago by atagar

Hi Ravi. It makes sense to me for get_circuits() to parse the output as CircuitEvents - the control spec defines the output as being circuit events after all. It might make sense to rename it to be something like a "CircuitEntry" to make it source-agnostic, but I don't have a strong opinion about that.

I'm not sure what you mean by "I'm adding a pdb.set_trace() in handle_streamcreated to check if it's being run.". What precisely would you like help with?

comment:6 in reply to:  5 ; Changed 5 years ago by neena

Replying to atagar:

but I don't have a strong opinion about that.

Me too.

I'm not sure what you mean by "I'm adding a pdb.set_trace() in handle_streamcreated to check if it's being run.". What precisely would you like help with?

Stem isn't calling handle_streamcreated with stream events.

  • The external_ip method makes a request to the Tor SocksPort.
  • The stream doesn't get attached because __LeaveStreamsUnattached is on.
  • Stem doesn't call handle_streamcreated with the new stream event. (Pretty sure I'm doing something wrong here)
  • The stream doesn't get attached within the timeout, and external_ip's SOCKS request fails with an error code.

comment:7 Changed 5 years ago by neena

Umm, nevermind. Figured it out. Sorry.
/facepalm

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

Replying to neena:

Ravi,

but I don't have a strong opinion about that.

Me too.

Your get_circuits() method is probably the best way within the current code. I have written a similar method for a desktop controller and ended up parsing the get_info() response myself. At least your approach re-uses good, tested code.

I think the best long-term method would be to move the re-usable parts from CircuitEvent to (maybe) stem.response.CircuitEntry, then make CircuitEvent and Controller.get_circuit() into wrappers for the new function. See how stem.descriptor.router_status_entry works with NetworkStatusEvent and get_network_status(). But, this may not fit within your plans.

If you do the same treatment for get_info("stream-status"), my controller can become just a lightweight gui wrapping Stem. 8-)

Just as style feedback... Do we want to standardize on using circuit_id and stream_id as var names? I see that repurpose_circuit(), extend_circuit(), close_circuit(), and attach_stream() each use circuit or circuit_id.

comment:9 Changed 5 years ago by atagar

I think the best long-term method would be to move the re-usable parts from CircuitEvent to (maybe) stem.response.CircuitEntry, then make CircuitEvent and Controller.get_circuit() into wrappers for the new function.

Agreed.

Just as style feedback... Do we want to standardize on using circuit_id and stream_id as var names? I see that repurpose_circuit(), extend_circuit(), close_circuit(), and attach_stream() each use circuit or circuit_id.

Yup, we should standardize on circuit_id.

comment:10 in reply to:  9 Changed 5 years ago by robinson

Replying to atagar:

I think the best long-term method would be to move the re-usable parts from CircuitEvent to (maybe) stem.response.CircuitEntry, then make CircuitEvent and Controller.get_circuit() into wrappers for the new function.

Agreed.

I looked into how this might be accomplished. Short story: it gets very hairy, very fast with interactions between response.convert, response.getinfo.GetInfoResponse, events.Event, and (in this case) events.CircEvent.

So, Ravi, I like your current solution. Unless we see a performance problem (unlikely), just think of this as "casting" from a GETINFO response to CIRC events. 8-)

comment:11 Changed 5 years ago by atagar

Hi Ravi. What's the state of this?

comment:12 Changed 5 years ago by robinson

Ravi,

Could you finish any clean up you want on Controller.get_circuits(), make it available on a separate branch, and open a ticket for Damian to pull it? I have been using your new get_circuits() for a couple days and it works well.

comment:13 in reply to:  11 Changed 5 years ago by neena

Status: needs_informationneeds_review

Please review and merge my branch here. I also fixed some tests in that branch that currently fail with the ONLINE target (test_map_address and test_close_circuit).

Thanks.

comment:14 Changed 5 years ago by atagar

Resolution: implemented
Status: needs_reviewclosed

Looks great! Merged with some minor changes (*)...

https://gitweb.torproject.org/stem.git/commit/af6fd83f74f6f39981cb32e1f871421755edfdaa

The ONLINE target (especially this test) is really flaky. The two most common failures I've seen is...

  • Circuits fail to be created/extended around 1/7th of the time, breaking the test.
  • Even if we get to the end of the test asserting the ip sometimes fails...
======================================================================
FAIL: test_attachstream
----------------------------------------------------------------------
Traceback:
  File "/home/atagar/Desktop/stem/test/integ/control/controller.py", line 748, in test_attachstream
    self.assertEquals(exit_ip, ip)
AssertionError: '37.130.227.132' != '37.130.227.133'

----------------------------------------------------------------------
Ran 25 tests in 20.524s

This mostly happens with the Torland exits. Iirc there's no assurance that the exit's ip is the location that we'll actually exit from, and many large exits don't.

  • Sometimes when we attempt to call external_ip() it hangs. Then when I hit ctrl+c our tor instance is in some sort of bad state. The following SETEVENTS call fails with '[Errno 104] Connection reset by peer' and all further attempts to establish a controller connection fails with 'connection refused'.

The first couple could be addressed by adding retries to the test. The last I really haven't a clue, and changes I've made might have inadvertently fixed it (I haven't seen it the last dozen times I've run 'em).

I'd love to see a patch to add the retries but for the purposes of this ticket I think that we can safely call this done.

Cheers! -Damian

  • Ok, ok, maybe not so minor, but I really did mean to just revise and merge this branch! Then one thing led to another, and six hours later I kinda-sorta had a few more things added in.
Note: See TracTickets for help on using tickets.