Skip to content

Multithreading fixes#288

Closed
haizaar wants to merge 9 commits into
prometheus:masterfrom
haizaar:master
Closed

Multithreading fixes#288
haizaar wants to merge 9 commits into
prometheus:masterfrom
haizaar:master

Conversation

@haizaar

@haizaar haizaar commented Jul 11, 2018

Copy link
Copy Markdown
Contributor

Fixes #287

@haizaar

haizaar commented Jul 11, 2018

Copy link
Copy Markdown
Contributor Author

@brian-brazil
What am I to do about DCO for my past commits?

Comment thread prometheus_client/core.py
class _Timer(object):
def __init__(self, callback):
self._callback = callback
self._storage = local()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe for reentrant use within the same thread?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will bring us back to the original problem when a single instance of timer used in different threads.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reentrant means that the function calls itself, e.g.:

def f():
  @s1.time():
     f()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@brian-brazil

Copy link
Copy Markdown
Contributor

All commits needs the DCO, it's easiest to squash.

Signed-off-by: Zaar Hai <haizaar@haizaar.com>
@haizaar

haizaar commented Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

Updated the code. Lets converge to LGTM and then I'll fix the DCO.

haizaar added 2 commits July 12, 2018 18:37
Signed-off-by: Zaar Hai <haizaar@haizaar.com>
Signed-off-by: Zaar Hai <haizaar@haizaar.com>
@haizaar

haizaar commented Jul 20, 2018

Copy link
Copy Markdown
Contributor Author

Closing in favor of #290

@haizaar haizaar closed this Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants