Implement Controller.attach_stream
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.
Activity
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.
Trac:
Status: new to needs_revision
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?
Trac:
Username: robinson
Status: needs_revision to needs_informationDamian, 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.
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?
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.
-
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.
Trac:
Username: robinson 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.
-
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-)
Trac:
Username: robinson 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.524sThis 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.
Trac:
Resolution: N/A to implemented
Status: needs_review to closed-
mentioned in issue #7832 (closed)