Remove some uses of IOLoop._running #1512
Conversation
…ove_some_running
Most tests will not stop the IOLoop. It is ok if they do or if they don't. |
| thread.start() | ||
| loop_started = threading.Event() | ||
| loop.add_callback(loop_started.set) | ||
| loop_started.wait() |
There was a problem hiding this comment.
This pattern seems much cleaner than our other approaches. We might consider doing this in client.py as well.
There was a problem hiding this comment.
Yes, I think I can refactor a bit more code in that PR.
distributed/utils.py
Outdated
| Otherwise, just mark this object "stopped". | ||
| """ | ||
| # XXX it would be more logical to stop an user-created loop | ||
| # if we started it, but that would break the current distributed API |
There was a problem hiding this comment.
In which situations would this break user code? Some breakage may be acceptable.
There was a problem hiding this comment.
Well, the problem is it currently breaks some of our tests. For example this one:
def test_Current(loop):
with cluster() as (s, [a, b]):
with Client(s['address'], loop=loop) as c:
assert Client.current() is c
with pytest.raises(ValueError):
Client.current()
with Client(s['address'], loop=loop) as c:
assert Client.current() is cAs soon as the first with ends, the loop is closed, but it's passed again to the second Client instead, so the test hangs.
I don't know if there are use cases for continuing to use a loop after you closed the client that started it, but this is how it currently works.
(of course, that test doesn't need to re-use the same loop, but since it's written that way, it seems to hint that it's a valid idiom...)
There was a problem hiding this comment.
If this is inconvenient then it might be reasonable to drop it. The next version will likely be a 0.x.0 bump, so we can break a few things.
There was a problem hiding this comment.
It's not technically inconvenient, it just seems a bit inconsistent to have such assymetry in the start/stop responsabilities. But it probably wouldn't save any complexity to make it different.
There was a problem hiding this comment.
If you think it's a good idea then go for it.
|
Will merge if no further comments. |
|
The one crucial remaining use of |
|
That looks like a rarely used code branch to me. Running tests now to see
where it comes up.
…On Wed, Nov 1, 2017 at 5:45 PM, Antoine Pitrou ***@***.***> wrote:
The one crucial remaining use of IOLoop._running is in the sync()
function. I haven't found a reliable way to do without it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1512 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASszFjkj_baJN9WLDTCNgB8aosLRTytks5syOaXgaJpZM4QM0e7>
.
|
|
I think that we can just remove this functionality. I suspect that it does not come up in practice. |
Actually, it comes up in the |
Also some small cleanups.
One question revolves around the
loopfixture. It asks the loop to stop at the end in case the test didn't stop already... and it turns out some tests don't seem to stop the loop indeed. Is it an error for a test not to do so?