Multithreading fixes#288
Conversation
Required for multithreading tests.
It's a 2.7 version of Python lang.
|
@brian-brazil |
| class _Timer(object): | ||
| def __init__(self, callback): | ||
| self._callback = callback | ||
| self._storage = local() |
There was a problem hiding this comment.
Is this safe for reentrant use within the same thread?
There was a problem hiding this comment.
You have a point here - local() returns reference to the same object within a thread and therefore it will work incorrectly, particularly for nested timers or regular timers in async code. I'm thinking about using id(self) as a local storage key.
What do you think?
There was a problem hiding this comment.
If id(self) would work, then we would be able to store in the self directly as it's just the memory address of self.
There was a problem hiding this comment.
It will bring us back to the original problem when a single instance of timer used in different threads.
There was a problem hiding this comment.
My point is that id(self) doesn't help, as you wouldn't be seeing a race in the first place if that worked. So this doesn't fix reentrancy. We may need a stack here.
There was a problem hiding this comment.
OK, lets define the problem :)
Having
s1 = Summary("s1",...)
s2 = Summary("s2",...)
The original issue is that:
@s1.time()
def f(): pass
Produces wrong timings when f() is called simultaneously from different threads. My last code solves inter thread issue using TLS, and uses id(self) as TSL key to support multiple timer instances within a thread. Such that both f() and g() (below) work
@s1.time()
def g():
s2.time()(lambda : time.sleep(1))()
And both f() and g() cases are covered with tests.
Now, what do you refer as reentrancy? This? -
@s1.time()
def h():
@s1.time()
def i(): pass
i()
This will work as well, since each call to time() will produce different instances of the _Time() with distinct id(self).
P.S. I actually think that the better and simpler idea is to rework both decorator and context manager to use function closure to record the start time. It will be also a bit faster IMHO. I need to think whether its possible to implement it without changing the time() semantics.
There was a problem hiding this comment.
Reentrant means that the function calls itself, e.g.:
def f():
@s1.time():
f()
There was a problem hiding this comment.
What's the use case?
Recursion like this?
@s1.time()
def f(i=0):
f(i+1) if i > 5 else True
Then no, _Timer is not reentrant. But neither is the current implementation (as in master).
|
All commits needs the DCO, it's easiest to squash. |
Signed-off-by: Zaar Hai <haizaar@haizaar.com>
|
Updated the code. Lets converge to LGTM and then I'll fix the DCO. |
Signed-off-by: Zaar Hai <haizaar@haizaar.com>
Signed-off-by: Zaar Hai <haizaar@haizaar.com>
|
Closing in favor of #290 |
Fixes #287