Implement capacity limitation for run_in_worker_thread #181

njsmith · 2017-05-29T09:02:20Z

You can pass a Semaphore here to avoid overwhelming the system with
too many simultaneous threads.

codecov · 2017-06-18T08:38:17Z

Codecov Report

Merging #181 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   99.06%   99.11%   +0.05%     
==========================================
  Files          63       63              
  Lines        8420     8745     +325     
  Branches      609      629      +20     
==========================================
+ Hits         8341     8668     +327     
+ Misses         62       61       -1     
+ Partials       17       16       -1

Impacted Files	Coverage Δ
trio/_threads.py	`100% <100%> (ø)`	⬆️
trio/tests/test_threads.py	`100% <100%> (ø)`	⬆️
trio/_sync.py	`100% <100%> (ø)`	⬆️
trio/tests/test_sync.py	`100% <100%> (ø)`	⬆️
trio/tests/test_util.py	`100% <0%> (ø)`	⬆️
trio/_util.py	`90% <0%> (+2.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6311069...9f21a04. Read the comment docs.

- New synchronization primitive: CapacityLimiter. Like a Semaphore but more specialized. See python-triogh-182 for rationale. - Add limiter= argument to run_in_worker_thread, that allows one to correctly (modulo python-trio#6 (comment)) control the number of active threads. - Added new function current_default_worker_thread_limiter(), which creates or returns a run-local CapacityLimiter, and made run_in_worker_thread use it by default when no other limiter= is given. Closes: python-triogh-10, python-triogh-57, python-triogh-156

njsmith · 2017-06-18T08:42:25Z

I just pushed a new, much-expanded version of this that I think finishes solving the basic don't-run-too-many-threads-at-once problem, and I think it's ready for review if anyone wants to take a look.

kyrias

Looks good overall, and probably more useful than the old version!

kyrias · 2017-06-18T20:11:45Z

trio/_sync.py

+
+        """
+        if borrower in self._borrowers:
+            raise RuntimeError(


Shouldn't this also be in the docstring?

kyrias · 2017-06-18T20:25:01Z

trio/_threads.py

+    def report_back_in_trio_thread_fn(result):
+        print("in trio thread", result)
+        def do_release_then_return_result():
+            print("asdF")


Fixes python-triogh-231

Fixed

njsmith · 2017-06-22T08:04:52Z

@kyrias: Addressed those issues, and also added some docs and a few more tests.

Let's see what Travis makes of this. The last run where everything froze on PyPy makes me very nervous. I can't reproduce it locally, but it seems too much too be a coincidence that both PyPy runs froze inside the thread tests on a PR dealing with the thread code...

njsmith · 2017-06-22T09:35:27Z

Welp, this appears to be a PyPy bug: https://bitbucket.org/pypy/pypy/issues/2591/closed-over-variable-updates-made-in-one

I think we might need to drop PyPy support in our CI until this gets fixed. But for now I'm going to bed, and we'll see if @arigo has magically fixed it by the time I wake up :-)

Carreau · 2017-06-22T11:30:19Z

You can set it in an allow_faillure section in travis.ci. it still tests but does not mark the GitHub status a red.

…

On Jun 22, 2017 11:35, "Nathaniel J. Smith" ***@***.***> wrote: Welp, this appears to be a PyPy bug: https://bitbucket.org/pypy/ pypy/issues/2591/closed-over-variable-updates-made-in-one I think we might need to drop PyPy support in our CI until this gets fixed. But for now I'm going to bed, and we'll see if @arigo <https://github.com/arigo> has magically fixed it by the time I wake up :-) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUezzBFYrmX8z2iXUovwHQWTyRinwRVks5sGjVggaJpZM4NpEOK> .

This is bad. The test is totally valid, and we should be running it on PyPy. But it's hitting this PyPy bug: https://bitbucket.org/pypy/pypy/issues/2591/ This commit should be reverted after PyPy fixes that bug.

njsmith · 2017-06-23T22:33:21Z

I just pushed a commit that skips the test for now on PyPy. I'm not super happy about this, because the test is valid and important. But this is an important feature that I don't want to get blocked by PyPy, and this has a few advantages over allow-failures:

With allow-failures, no-one will notice if other stuff breaks on pypy
In fact, probably no-one will ever look at the pypy results at all, because when a PR is green then who clicks on the detailed results?
And the pypy bug was causing freezes and then timing out after 10 minutes, which substantially increases travis's total turnaround time.
I version-gated the skip logic, so even if we forget about it then eventually a new version of pypy will come along and start running the test again

njsmith · 2017-06-24T04:09:34Z

Here's an exciting plot twist: the pypy bug where local variable assignments can just disappear, also affects all versions of CPython, probably back to 2.1 when closures were introduced! bpo-30744

It just happens that we don't trigger it because one of the required conditions is the presence of a Python-level tracing function, which on PyPy is provided by coverage.py, but on CPython coverage.py has a C accelerator module.

Basically using nonlocal and threads together is super dangerous. - There was a weird/harmless one in _await_in_trio_thread, which caused me to clean up this code a bit. (It's better to use disable_ki_protection as a real decorator, in case it switches to mutating its argument in the future...) - Since test_run_in_worker_thread_limiter is actually doing something that's dangerous on both CPython and PyPy and the upstream fix is not obvious, let's just avoid nonlocal entirely.

kyrias · 2017-06-26T14:10:05Z

trio/_threads.py

+    def unprotected_fn():
+        return fn(*args)
+    res = _core.Result.capture(unprotected_fn)
+    print(res)


I wonder if it would make sense to add some off-by-default logging to various parts of trio, hooking into the logging module or somesuch. Might help with debugging things in general.

kyrias · 2017-06-26T14:11:50Z

Wowza, that's pretty nasty behavior indeed. AFAICT the code does look sane at least, though I haven't actually played with it yet.

njsmith · 2017-06-26T21:33:12Z

I'm going to call that a review and merge this. This has been way more of a roller coaster than I expected and I want to get it landed before another surprise pops up :-).

(Note for posterity: since it turns out that the test that broke PyPy is also fragile on all versions of CPython, I rewrote it to avoid using nonlocal and then reenabled it everywhere.)

This was referenced Jun 11, 2017

add asynchronous file io and path wrappers #180

Merged

Deadlock detection #182

Open

njsmith force-pushed the thread-limiter branch from a902bd0 to 2cf6081 Compare June 18, 2017 08:38

njsmith force-pushed the thread-limiter branch from 2cf6081 to 7d2d571 Compare June 18, 2017 08:40

njsmith changed the title ~~Add limiter= argument to run_in_worker_thread~~ Implement capacity limitation for run_in_worker_thread Jun 18, 2017

Hook CapacityLimiter up to the generic lock testing machinery

d9d6ec5

kyrias previously requested changes Jun 18, 2017

View reviewed changes

njsmith added 3 commits June 21, 2017 01:15

Document that CapacityLimit.acquire* can raise RuntimeError

4350903

Remove some stray print() calls

315bb7f

Add conceptual docs for run_in_worker_thread

f5c4911

Fixes python-triogh-231

Skip test_run_in_worker_thread_limiter on PyPy

770f1fc

This is bad. The test is totally valid, and we should be running it on PyPy. But it's hitting this PyPy bug: https://bitbucket.org/pypy/pypy/issues/2591/ This commit should be reverted after PyPy fixes that bug.

njsmith mentioned this pull request Jun 23, 2017

Design: thread pools #6

Closed

njsmith added 2 commits June 24, 2017 02:51

Add a pragma: no cover

9f21a04

kyrias reviewed Jun 26, 2017

View reviewed changes

njsmith merged commit 1d0a764 into python-trio:master Jun 26, 2017

njsmith mentioned this pull request Aug 8, 2017

Give run_in_worker_thread a semaphore= argument #57

Closed

njsmith deleted the thread-limiter branch August 8, 2017 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Implement capacity limitation for run_in_worker_thread #181

Implement capacity limitation for run_in_worker_thread #181

Uh oh!

njsmith commented May 29, 2017

Uh oh!

codecov bot commented Jun 18, 2017 •

edited

Loading

Uh oh!

njsmith commented Jun 18, 2017

Uh oh!

kyrias left a comment

Uh oh!

kyrias Jun 18, 2017

Uh oh!

kyrias Jun 18, 2017

Uh oh!

njsmith commented Jun 22, 2017

Uh oh!

njsmith commented Jun 22, 2017

Uh oh!

Carreau commented Jun 22, 2017 via email

Uh oh!

njsmith commented Jun 23, 2017

Uh oh!

njsmith commented Jun 24, 2017

Uh oh!

kyrias Jun 26, 2017

Uh oh!

kyrias commented Jun 26, 2017

Uh oh!

njsmith commented Jun 26, 2017

Uh oh!

Uh oh!

Uh oh!

Implement capacity limitation for run_in_worker_thread #181

Implement capacity limitation for run_in_worker_thread #181

Uh oh!

Conversation

njsmith commented May 29, 2017

Uh oh!

codecov bot commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

njsmith commented Jun 18, 2017

Uh oh!

kyrias left a comment

Choose a reason for hiding this comment

Uh oh!

kyrias Jun 18, 2017

Choose a reason for hiding this comment

Uh oh!

kyrias Jun 18, 2017

Choose a reason for hiding this comment

Uh oh!

njsmith commented Jun 22, 2017

Uh oh!

njsmith commented Jun 22, 2017

Uh oh!

Carreau commented Jun 22, 2017 via email

Uh oh!

njsmith commented Jun 23, 2017

Uh oh!

njsmith commented Jun 24, 2017

Uh oh!

kyrias Jun 26, 2017

Choose a reason for hiding this comment

Uh oh!

kyrias commented Jun 26, 2017

Uh oh!

njsmith commented Jun 26, 2017

Uh oh!

Uh oh!

codecov bot commented Jun 18, 2017 •

edited

Loading