Is this kitchen timer code pythonic?

I've been doing bits and bobs with Python for a few years, but I've never took the time to get feedback on what's pythonic and what's not. In the interest of collaborating more with Python programmers, I'd like to make sure my code follows conventions and idioms that the majority of Pythonistas will recognize.

Please review my Kitchen Timer class for:

• pythonic idioms and conventions

• behavioural correctness (especially around multi-threading and timing precision)

• portability

• readability

• maintainability

• efficiency

Also, if the purpose of the class is unclear from its API and comments, please offer your suggestions for improvement there too.

```
from threading import Timer as TTimer
from time import time
from threading import Lock

class NotRunningError(Exception): pass
class AlreadyRunningError(Exception): pass

class KitchenTimer(object):
'''
Loosely models a clockwork kitchen timer with the following differences:
You can start the timer with arbitrary duration (e.g. 1.2 seconds).
The timer calls back a given function when time's up.
Querying the time remaining has 0.1 second accuracy.
'''

running = "running"
stopped = "stopped"
timeup = "timeup"

def __init__(self):
self.__stateLock = Lock()
self.state = self.stopped
self.__timeRemainingLock = Lock()
self.timeRemaining = 0

def start(self, duration=1, whenTimeup=None):
if self.isRunning():
raise AlreadyRunningError
else:
self.state = self.running
self.timeRemaining = duration
self._userWhenTimeup = whenTimeup
self._startTime = self._now()
self._timer = TTimer(duration, self._whenTimeup)
self._timer.start()

def stop(self):
if self.isRunning():
self._timer.cancel()
self.state = self.stopped
self.timeRemaining -= self._elapsedTi

1. Bugs

-
You have two locks, one protecting state, and the other protecting timeRemaining. But these locks operate independently. That means that when you update both of these properties:

self.state = self.timeup
self.timeRemaining = 0

it would be possible for another thread to run in between these two statements and see state == timeup but timeRemaining != 0. You need one lock to protect both properties, so that they are updated atomically. See below for some race conditions that are consequences of this bug.

-
The timeRemaining property updates the _timeRemaining value by subtracting the elapsed time. But this means that if you keep requesting the timeRemaining property, it will keep subtracting the elapsed time, giving you nonsense like this:

>>> t = KitchenTimer()
>>> t.start(5)
>>> t.timeRemaining
3.2
>>> t.timeRemaining
0.6
>>> t.timeRemaining
-2.6
>>> t.timeRemaining
-6.6
>>> t.timeRemaining
-11.3
>>> t.timeRemaining
0.0

-
There's a race condition between the start method and the whenTimeup method. In start you have:

if self.isRunning():                  # 1
    raise AlreadyRunningError    
else:
    self.state = self.running         # 2
    self.timeRemaining = duration     # 3

and in _whenTimeUp you have:

self.state = self.timeup              # 4
self.timeRemaining = 0                # 5

A possible order of execution is 4–1–2–3–5 which would set state to running and timeRemaining to 0.

-
There's a race condition between the stop method and the _whenTimeup method. In stop you have:

if self.isRunning():                  # 1
    self._timer.cancel()              # 2
    self.state = self.stopped         # 3

and in _whenTimeUp you have:

self.state = self.timeup              # 4

A possible order of execution is 1–4–2–3 which would leave state as stopped when it ought to be timeup.

(There may well be more race conditions; I haven't looked very hard.)

1. Comments on your code

-
Why "Kitchen" timer? The class doesn't seem to have anything to do with kitchens.

-
There doesn't seem to be any documentation for the methods.

-
Why does the timeRemaining method round the result to one decimal place? There doesn't seem to be any explanation of this. It would be more natural to return the actual time remaining.

-
Why do you rename threading.Timer as TTimer? The name Timer doesn't seem to conflict with anything in your code.

-
The Python style guide (PEP8) recommends using lowercase_with_underscores for method and property names. You're not obliged to follow this guide, but it will make it easier for you to collaborate with other Python programmers.

-
The intended use of private names in Python is to "avoid name clashes of names with names defined by subclasses" but that's not necessary here. So write _state_lock instead of __stateLock and so on.

-
The _now method seems a bit pointless:

def _now(self):
    return time()

Why not just call time()?

StackExchange Code Review Q#33113, answer score: 5