stem.socket.ControlSocket.is_alive() should check if socket is open explicitly

"Checks if the socket is known to be closed. We won't be aware if it is until we either use it or have explicitily shut it down." Instead of this we should do a sample query and if we get a SocketError, then return a False.

def is_alive(self):
  try:
    self.send("GETINFO version")
   except SocketError:
     self._is_alive = False
  return self._is_alive

added component::archived/stem owner::atagar priority::medium resolution::implemented status::closed type::defect labels

The ControlSocket's is_alive() method is very frequently used, both internally and by Controllers, to test for socket health. Making this perform a query would be disastrous in terms of load. It would also break if we weren't yet authenticated.

The caveat that the is_alive() method places, about knowing when a socket's alive or not, is mitigated by the BaseController because it continually pulls from the socket and hence does know when the socket is no longer alive...

This means that to have reliable detection for when we're disconnected
you need to continually pull from the socket (which is part of what the
:class:`stem.control.BaseController` does).

https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/socket.py#l142

Feel free to reopen if you still think that we should proceed with this for some reason.

Trac:
Status: new to closed
Resolution: N/A to not a bug

The is_alive() method is broken when a controller issues a "QUIT" message.

>> controller.msg("QUIT")
closing connection
>> controller._socket.is_alive()
True #Should be False

Do we want this to happen? Maybe we can use _is_alive() for internal purposes which uses __is_alive boolean and another is_alive() which actually checks the socket with a query and expose this as the end-user API? We can use a sample PROTOCOLINFO query to check - it doesn't need auth, so it won't close the connection

Trac:
Resolution: not a bug to N/A
Status: closed to reopened

Oops wikiformatting messed it up, rewriting previous comment -

Do we want this to happen? Maybe we can use _is_alive() for internal purposes which can use a __is_alive boolean and another _is_alive() which actually checks the socket with a query and expose this as the end-user API? We can use a sample PROTOCOLINFO query to check - it doesn't need auth, so it won't close the connection

Trac:
Status: reopened to needs_information

The is_alive() method is broken when a controller issues a "QUIT" message.

Hm, that does seem like a bug. I'll look into it a bit tomorrow and add a test for it - not sure what's happening there yet.

The controller's is_alive() method should behave the way that you want, and it's a bug that it doesn't. We shouldn't need to actively query tor to make this work, though.

We can use a sample PROTOCOLINFO query to check - it doesn't need auth, so it won't close the connection

Funny thing about PROTOCOLINFO, it does disconnect the socket if you issue it more than once before authenticating. Personally I think that's stupid (and had to hack around it in the connection module), but oh well.

Replying to atagar:

Hm, that does seem like a bug. I'll look into it a bit tomorrow and add a test for it - not sure what's happening there yet.

FYI, I've added a Controller.quit() and a test(which fails now)- https://github.com/gsathya/stem/commit/6fa3e878829b4f0107e8b34a42be2f879a6a73f9

>> controller.msg("QUIT")
closing connection
>> controller._socket.is_alive()
True #Should be False

Tried this on linux and it worked. Not sure if this is another bit of mac suckyness, I'll try it there tomorrow.

FYI, I've added a Controller.quit() and a test(which fails now)- https://github.com/gsathya/stem/commit/6fa3e878829b4f0107e8b34a42be2f879a6a73f9

Oooh, a github repo. I was confused for a bit about why I wasn't finding this change. Are you planning to do most of your future work in your tpo or github repo? It makes it simpler if I have a single place to pull from. ;)

atagar@morrigan:~/Desktop/stem$ git config --list | grep gsathya
remote.gsathya.url=git://git.torproject.org/user/gsathya/stem.git
remote.gsathya.fetch=+refs/heads/*:refs/remotes/gsathya/*

Replying to atagar:

Oooh, a github repo. I was confused for a bit about why I wasn't finding this change. Are you planning to do most of your future work in your tpo or github repo? It makes it simpler if I have a single place to pull from. ;)

Heh sorry, I pushed that from my office laptop so I didn't have my SSH keys with me. I'll use tpo :)

 self.assertRaises(stem.socket.SocketClosed, controller.protocolinfo)

controller.protocolinfo never raises SocketClosed, because "If the socket is already closed then it is first reconnected".

 self.assertEqual(controller._socket.is_alive(), False)

Strange race condition, adding a time.sleep(1) before the assertion makes it pass. Not sure why this is happening. This looks like a bug in how is_alive() works.

Trac:
Cc: gsathya to gsathya, neenaoffline@gmail.com

That makes sense. The is_alive() method works as follows...

• is_alive() simply provides the cached is_alive status (ie, what do we think we are?)
• our is_alive flag toggles when connect() or close() is called
• our close() is called when we attempt to read from our socket and it reports that the socket is closed

So, when you call 'QUIT' the controller sends the message, receives an OK reply then returns to the caller. Meanwhile the reader loop pulls the socket again and (possibly delayed by tor or the os) sees that our socket is shut down and calls close() to clean up.

Obviously to make the 'QUIT' case work we could simply call close() ourselves before returning. It would be a bug if our is_alive() state didn't change for a significant amount of time, but is_alive() just reflects 'do I seem to be alive at this instant'. We could acquire the read lock to narrow the gap, but that doesn't seem like a good idea and wouldn't completely eliminate this "'QUIT' -> is_alive() is false" gap.

So in summary, I think that having our quit() method call close() is probably the best option. Let me know if you strongly disagree.

Ping? Did you want to discuss this more?

One other option that I'd be fine with is a 'test_connection' argument for is_alive() which does an active ping check. That said, this would require a change on tor's side to accept a 'PING' request without disconnecting the socket if we haven't yet authenticated.

Replying to atagar:

One other option that I'd be fine with is a 'test_connection' argument for is_alive() which does an active ping check. That said, this would require a change on tor's side to accept a 'PING' request without disconnecting the socket if we haven't yet authenticated.

I like this. Filed a ticket - https://trac.torproject.org/projects/tor/ticket/6354

Should we do something about this meanwhile?

Stem's Controller class has an is_alive() method which simply calls its socket's is_alive(). You're welcome to add a 'test_connection' argument (defaulted to False) which does a 'GETINFO version' check. This, of course, will be problematic if we aren't yet authenticated but you can note that in the pydocs.

If you go with this then please cite this ticket via a comment in the code so we know that there may be a PING method in the future.

I don't really want to add a test_connection, especially with a get_version which will be problematic. I'm pretty much satisfied with this for now. Feel free to change it. https://gitweb.torproject.org/user/gsathya/stem.git/commitdiff/04a1e99e4310adc3bce6fbda9860815a089753c7?hp=6fa3e878829b4f0107e8b34a42be2f879a6a73f9

Thanks!

Trac:
Status: needs_information to needs_review

self.msg("QUIT")

We should check the QUIT's reply. You'll want to use Ravi's SingleLine response parser...

response = self.msg("QUIT")
stem.response.convert("SINGLELINE", response)

if response.is_ok():
  self.close()
else:
  raise stem.socket.ProtocolError("The QUIT provided a non-ok response: %s" % response)

• with runner.get_tor_controller(False) as controller:

•  controller.quit()

•  self.assertEqual(controller._socket.is_alive(), False)

•  self.assertRaises(stem.socket.SocketClosed, controller.authenticate(test.runner.CONTROL_PASSWORD))

Lets test with both an authenticated an unauthenticated controller. Amusingly this test would also work if you called 'controller.msg("GOBBDLYGOOK")' followed by a sleep since unauthenticated sockets will disconnect us anyway.

Also, please use get_socket() rather than accessing the _socket member directly and go back to using controller.protocolinfo() rather than authenticate (does the same thing and it's less verbose).

Otherwise looks good!

========================================

Slightly weird idea, but instead of adding a quit() method what about adding a best-effort msg("QUIT") call to our close() method instead? Both quit() and close() do largely the same thing, and users don't really care if the QUIT call succeeds or not as long as the socket is closed (we can add a flag if that isn't the case).

Trac:
Status: needs_review to needs_revision

Replying to atagar:

https://gitweb.torproject.org/user/gsathya/stem.git/commitdiff/cf31ed66aa7cc2eb6fac833f44b7f0c6f00e059b?hp=1e2bdc4b8f2c841a41b7b73c5b4fe9c9926deedd

We should check the QUIT's reply. You'll want to use Ravi's SingleLine response parser...

Done

Lets test with both an authenticated an unauthenticated controller. Amusingly this test would also work if you called 'controller.msg("GOBBDLYGOOK")' followed by a sleep since unauthenticated sockets will disconnect us anyway.

Won't work. It'll just create a new socket and execute the command - it won't fail. I'm happy with just socket.is_alive() returning False.

Also, please use get_socket() rather than accessing the _socket member directly

Done. Although, this reminds me of Java. Accessor functions in python?

Slightly weird idea, but instead of adding a quit() method what about adding a best-effort msg("QUIT") call to our close() method instead?

I like it. Done.

PS - I think I messed up this git branch, you might have to do some rebasing or some other git foo. Sorry

Trac:
Status: needs_revision to needs_review

Slightly weird idea, but instead of adding a quit() method what about adding a best-effort msg("QUIT") call to our close() method instead?

I like it. Done.

Most of my comments were no longer relevant if we went this route (in which case we simply make a best-effort QUIT attempt). Merged a simple change that does this...

https://gitweb.torproject.org/stem.git/commitdiff/e2528b85f6b0bf1fb82c42599e71d1c925fcd6ae

Feel free to reopen if there's something more that you think that we should do.

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed