#17946 closed defect (fixed)

launch_tor should kill process on exception while bootstrapping (for example, on KeyboardInterrupt)

Reported by: germn Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Normal Keywords: launch_tor, bootstrap
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

What will happen if we will call

stem.process.launch_tor_with_config(config, timeout=None)

and CTRL+C some time after?

KeyboardInterrupt exception would be raised somewhere inside launch_tor, and we will have one of following situations:
1) KeyboardInterrupt raised before or while subprocess.Popen - everything is ok
2) KeyboardInterrupt raised after subprocess.Popen, inside bootstrapping, but before launch_tor finished - we will have hanging tor process with no ability to terminate it manually.

This issue is about to fix second case. All we need is to add handling of second situation inside launch_tor function. Here's example of code that should be added before line 165 inside stem.process:

except:  # on any exception while bootstrapping
    tor_process.kill()  # kill existing process
    raise  # reraise exception

But note, you should also fix "finally" block to prevent closing stdout, stderr of terminated process.

Note, this issue is not only about KeyboardInterrupt, but also for any other exception that can be raised suddenly everyehere, like asyncio.CancelledError

Child Tickets

Change History (3)

comment:2 Changed 21 months ago by germn

Hello, thanks for answer!

I didn't understent this part:

  except:
    if tor_process:
      tor_process.kill()  # don't leave a lingering process
      tor_process.wait()

    exc = sys.exc_info()[1]

    if type(exc) == OSError:
      raise  # something we're raising ourselves
    else:
      raise OSError('Unexpected exception while starting tor (%s): %s' % (type(exc).__name__, exc))

If KeyboardInterrupt (or asyncio.CancelledError, or other) was raised, we should reraise exactly this error after resources cleanup, we shouldn't raise our type of error instead. Top-level code might want to know if exactly KeyboardInterrupt happened.

Here's little syntetic example that demonstrates what I mean:

import time
import sys


def launch_tor_mock():
    try:
        time.sleep(5)  # emulate bootstrapping
    except:
        exc = sys.exc_info()[1]

        if type(exc) == OSError:
          raise  # something we're raising ourselves
        else:
          raise OSError('Unexpected exception while starting tor (%s): %s' % (type(exc).__name__, exc))


if __name__ == '__main__':
    try:
        launch_tor_mock()
    except KeyboardInterrupt:
        print('Dear user, you\'ve just pressed CTRL+C, don\'t do it!')
    except:
        print('Dear user, some tor process error you couldn\'t prevent happend.')

If you'll run it and press CTRL+C, you'll see second msg that's wrong.

I think we should just reraise error, without thinking what type it is:

  except:
    if tor_process:
      tor_process.kill()  # don't leave a lingering process
      tor_process.wait()
    raise  # just reraise exception we got

I understand about docstring ":raises", but you can't forecast any type of error code can raise in special cases. KeyboardInterrupt is one of this type of "unexpected" exceptions that can be raised anywhere.

...

One another little thing is about line 141:

      # this will provide empty results if the process is terminated
      if not init_line:
        raise OSError('Process terminated: %s' % last_problem)

Here was process killing. I'm not sure, but may be better is to keep it? At least in case this commit is about another thing...

      # this will provide empty results if the process is terminated
      if not init_line:
        if tor_process:  # ... but best make sure
          tor_process.kill()
          tor_process.wait()
        raise OSError('Process terminated: %s' % last_problem)

comment:3 Changed 21 months ago by atagar

Resolution: fixed
Status: newclosed

If KeyboardInterrupt (or asyncio.CancelledError, or other) was raised, we should reraise exactly this error after resources cleanup, we shouldn't raise our type of error instead. Top-level code might want to know if exactly KeyboardInterrupt happened.

Yup, I can see an argument for this. Reason I did this was because launch_tor() is only documented as raising OSErrors. I'm unfamiliar with asyncio.CancelledError, but you're right that KeyboardInterrupt is an interesting critter. If the caller explicity invokes an exception within us then I could see an argument for re-raising that. Changed.

One another little thing is about line 141:

I removed a couple tor_process.kill() calls since they're now covered by our except block. No need to kill the process twice. :)

Feel free to reopen if ya spot anything else.

Note: See TracTickets for help on using tickets.