Removing most of inlineCallbacks for the sake of cancellation support #191

IlyaSkriblovsky · 2016-10-08T22:27:44Z

inlineCallbacks is totally cool with one drawback: it doesn't properly supports .cancel(). If you cancel inlineCallbacks-based function it will silently continue to work and just ignore the result. Currently there is no any way for external code to stop inlineCallbacks in the middle of its execution.

On the other hand, plain old Deferreds are rather good at cancellation: when all the call stack is written in plain addCallback-style, .cancel() is able to stop execution at any stage without any explicit support from the code.

So, I suggest to get rid of almost all inlineCallbacks in txmongo stack to make it possible to cancel all DB operations.

It should be noted though that cancel() will have limited usefulness because in normal operation txmongo sends requests to DB immediately and such queries can't be really cancelled. .cancel() can only prevent execution of queries that was held back by waiting for active connection in the pool or complex multi-stage operations like query with getmore's or bulk writes.

Other minor advantage of removing inlineCallbacks is that it takes considerably more RAM, up to ×3 times according to my benchmarks. In app with tons of concurrent queries it may result in performance increase under high loads (high memory usage calls GC more often).

This PR changes a lots of code and also replaces some nice yield-based loops with tricky recursive callbacks. So it needs careful code review!

If this PR will be merged, we can eventually get rid of @timeout and check_deadline() in favor of upcoming Deferred.addTimeout() (twisted/twisted@f4b3869)

Remaining issues:

find() should kill cursor if cancelled between loading result batches

Questions to discuss:

I've added simple cancellers for Deferreds, created by txmongo.protocol and txmongo.connection that simply unregister deferred from the waiting lists. Such cancellers are actually useless, because they are effectively identical to Deferreds without cancellers (because Deferred without canceller have special semantics: they are just swallowing .callback() call after cancellation and doesn't call the callback chain, so effect would be the same). Should I keep them or its better to remove them?
Functions that were inlineCallback-based, previously returned failed deferreds if arguments didn't passed checks. They are now raising argument checking errors directly, without wrapping with Deferred. Hence some assertFailure was replaced by assertRaises in testing code. This is actually the backwards-incompatible change. Should I wrap these TypeErrors with defer.fail() to make code backwards-compatible?

coveralls · 2016-10-08T22:56:02Z

Coverage decreased (-0.2%) to 95.374% when pulling d5e8da9 on IlyaSkriblovsky:less-inlinecallbacks into 6225f42 on twisted:master.

trenton42 · 2016-10-09T01:17:14Z

I think this is a really good change. I haven't reviewed the code yet, but It seemed to me there were a lot of inlineCallbacks added for no reason at all. Quite a daunting task to day on, so high five to @IlyaSkriblovsky for taking that on!

IlyaSkriblovsky · 2016-10-09T09:59:20Z

@trenton42 Thanks! Code review would be very appreciated. I'm afraid I might be messing something with complex cases like bulk_write and related functions.

psi29a · 2016-10-10T07:04:50Z

Looks like something went bad in the last commit...

I'll give this a go over today at lunch. Same with other PR.

I would still like for there to be timeouts and deadlines, I'm hoping this will just help simplify the code base?

IlyaSkriblovsky · 2016-10-10T07:59:18Z

@psi29a, aah, sorry, there was a stupid mistake in last commit. Fixed.

Actualy, I don't much like current timeout implementation for two reasons:

It adds hidden implicit **kwargs to every query method. Such hidden arguments are not visible in the method signature, they are not handled properly by IDEs, they are error-prone for typos, etc. I think we should eventually eliminate almost all **kwargs and replace them by explicit keyword arguments. Explicit is better than implicit :) By the way, PyMongo already did it: before we were specifying write-concern, BSON options like as_class and other options via implicit kwargs which were passed back and forth thru PyMongo's internal functions. But in 3.x they moved all these general query options to explicit clean with_options and all new API methods introduced in 3.x are without kwargs.
Timing out Deferreds is a common task. Why do it inside txmongo when it can be done outside? Upcoming release of Twisted will introduce Deferred.addTimeout() which does roughly the same. So coll.find(..., timeout=10) might be converted to coll.find(...).addTimeout(10, reactor) and the latter seems cleaner and more universal. There is no addDeadline() though but it can be easily done with standalone function rather than mixing it inside every query method. @timeout was implemented that way because txmongo didn't supported cancellation and support was needed from check_deadline() calls inside query methods implementation. If this PR will be merged, same effect can be achieved by simply cancelling returned deferreds, without any special support from txmongo's internals. So, the task of timing out can be separated from txmongo's internals.

Anyway, even if you want to leave the current API, @timeout implementation could be shortened like this for modern Twisted version:

def timeout(func):
    def _timeout(*args, **kwargs):
        d = func(*args, **kwargs)
        timeout = calc_timeout_from_kwargs(kwargs)
        if timeout:
            d.addTimeout(timeout, reactor)
        return d
     return _timeout

(To support older versions, existing code with callLater() would be needed.)

coveralls · 2016-10-10T08:36:07Z

Coverage increased (+0.005%) to 95.587% when pulling 4c80a0f on IlyaSkriblovsky:less-inlinecallbacks into 6225f42 on twisted:master.

IlyaSkriblovsky · 2016-10-10T08:40:24Z

Tests against Twisted trunk are failing because today they have changed attributes of twisted._version (twisted/twisted@5c17777). I will create another PR to address this.

psi29a · 2016-10-10T11:39:30Z

The reasons why it was done inside of txmongo was because canceling deferreds with @inline.callbacks was an impossibility. ;) Chicken/Egg problem...

How much older do you mean, in terms of Twisted? Are we OK with Twisted >= 15.0

psi29a · 2016-10-10T11:42:37Z

In theory, now that we have cancelable deferreds, a lot of the timeout/deadline code can go away... along with that nasty hack.

IlyaSkriblovsky · 2016-10-10T11:51:44Z

This PR is meant to remove the Chicken :)

addTimeout is only in Twisted trunk, it is not released yet. So, I'm speaking mostly about future. Sorry, I wasn't clear about that :)

IlyaSkriblovsky · 2016-10-10T12:07:22Z

Currently, after accepting this PR we might only simplify the implementation of @timeout a bit by removing check_deadline() and hidden "_deadline" kwarg.

IlyaSkriblovsky · 2016-10-10T12:21:50Z

Rebased

coveralls · 2016-10-10T12:51:41Z

Coverage increased (+0.005%) to 95.587% when pulling dd70401 on IlyaSkriblovsky:less-inlinecallbacks into ce9636e on twisted:master.

psi29a · 2016-10-10T13:21:05Z

tests/test_collection.py

+        #self.assertFailure(self.db.test.find(spec="test"), TypeError)
+        #self.assertFailure(self.db.test.find(fields="test"), TypeError)
+        #self.assertFailure(self.db.test.find(skip="test"), TypeError)
+        #self.assertFailure(self.db.test.find(limit="test"), TypeError)


Why are these commented?

They are replaced by assertRaises below. Commented ones should be removed, just forgot it.

psi29a · 2016-10-10T13:21:40Z

tests/test_connection.py


-        d_insert = self.named_conn.db.coll.insert({'x': 42}, safe=True, timeout=-10)
-        yield self.assertFailure(d_insert, TimeExceeded)
+        self.assertRaises(TimeExceeded, self.named_conn.db.coll.insert, {'x': 42}, safe=True, timeout=-10)


Great! 👍

I'm not sure... Previously all exceptions from @inlineCallbacks-based methods were returned as failed Deferred. And calling code might be sure that it will always return Deferred. This PR changes this: for some errors it will still return failed Deferreds, but other errors might be raised immediately as direct exceptions. Hence all these changes in testing code.

This actually changes current API and potentially may break some existing code. (inlineCallbacks-based user code won't be affected).

Previous behavior can be easily saved by returning defer.fail(...) instead of raising, but it looks ugly.

What do you think?

psi29a · 2016-10-10T13:24:46Z

txmongo/collection.py

+
+        return self._database.command(SON([("listCollections", 1),
+                                     ("filter", {"name": self.name})]))\
+                   .addCallback(on_ok)


For readability, can you do a carriage return before SON to get it to the next line? Will this give us enough room to attach .addCallback(on_ok) without having to append an \ ?

coveralls · 2016-10-10T14:36:06Z

Coverage increased (+0.005%) to 95.587% when pulling 303c119 on IlyaSkriblovsky:less-inlinecallbacks into ce9636e on twisted:master.

psi29a · 2016-10-10T20:35:16Z

Accepting your Chicken removal code. :)

This has been along time coming. Waiting on travis re-build...

trenton42 · 2016-10-10T20:43:51Z

I wish I had more time to dive in and help out here. Thanks for this, @IlyaSkriblovsky 👏

trenton42 · 2016-10-10T20:44:43Z

And thanks @psi29a for reviewing 👏

IlyaSkriblovsky force-pushed the less-inlinecallbacks branch from 74d8784 to 4c80a0f Compare October 10, 2016 07:13

IlyaSkriblovsky added 2 commits October 10, 2016 15:19

Removing most of inlineCallbacks for the sake of cancellation support

7195660

Removed redundant checks in cancellers

dd70401

IlyaSkriblovsky force-pushed the less-inlinecallbacks branch from 4c80a0f to dd70401 Compare October 10, 2016 12:21

psi29a reviewed Oct 10, 2016

View reviewed changes

codestyle fix

303c119

psi29a merged commit d279627 into twisted:master Oct 10, 2016

IlyaSkriblovsky deleted the less-inlinecallbacks branch October 10, 2016 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing most of inlineCallbacks for the sake of cancellation support #191

Removing most of inlineCallbacks for the sake of cancellation support #191

IlyaSkriblovsky commented Oct 8, 2016 •

edited

Loading

coveralls commented Oct 8, 2016 •

edited

Loading

trenton42 commented Oct 9, 2016

IlyaSkriblovsky commented Oct 9, 2016

psi29a commented Oct 10, 2016

IlyaSkriblovsky commented Oct 10, 2016 •

edited

Loading

coveralls commented Oct 10, 2016 •

edited

Loading

IlyaSkriblovsky commented Oct 10, 2016

psi29a commented Oct 10, 2016

psi29a commented Oct 10, 2016

IlyaSkriblovsky commented Oct 10, 2016

IlyaSkriblovsky commented Oct 10, 2016

IlyaSkriblovsky commented Oct 10, 2016

coveralls commented Oct 10, 2016 •

edited

Loading

psi29a Oct 10, 2016

IlyaSkriblovsky Oct 10, 2016

psi29a Oct 10, 2016

IlyaSkriblovsky Oct 10, 2016

psi29a Oct 10, 2016

IlyaSkriblovsky Oct 10, 2016

coveralls commented Oct 10, 2016 •

edited

Loading

psi29a commented Oct 10, 2016 •

edited

Loading

trenton42 commented Oct 10, 2016

trenton42 commented Oct 10, 2016

Removing most of inlineCallbacks for the sake of cancellation support #191

Removing most of inlineCallbacks for the sake of cancellation support #191

Conversation

IlyaSkriblovsky commented Oct 8, 2016 • edited Loading

coveralls commented Oct 8, 2016 • edited Loading

trenton42 commented Oct 9, 2016

IlyaSkriblovsky commented Oct 9, 2016

psi29a commented Oct 10, 2016

IlyaSkriblovsky commented Oct 10, 2016 • edited Loading

coveralls commented Oct 10, 2016 • edited Loading

IlyaSkriblovsky commented Oct 10, 2016

psi29a commented Oct 10, 2016

psi29a commented Oct 10, 2016

IlyaSkriblovsky commented Oct 10, 2016

IlyaSkriblovsky commented Oct 10, 2016

IlyaSkriblovsky commented Oct 10, 2016

coveralls commented Oct 10, 2016 • edited Loading

psi29a Oct 10, 2016

Choose a reason for hiding this comment

IlyaSkriblovsky Oct 10, 2016

Choose a reason for hiding this comment

psi29a Oct 10, 2016

Choose a reason for hiding this comment

IlyaSkriblovsky Oct 10, 2016

Choose a reason for hiding this comment

psi29a Oct 10, 2016

Choose a reason for hiding this comment

IlyaSkriblovsky Oct 10, 2016

Choose a reason for hiding this comment

coveralls commented Oct 10, 2016 • edited Loading

psi29a commented Oct 10, 2016 • edited Loading

trenton42 commented Oct 10, 2016

trenton42 commented Oct 10, 2016

IlyaSkriblovsky commented Oct 8, 2016 •

edited

Loading

coveralls commented Oct 8, 2016 •

edited

Loading

IlyaSkriblovsky commented Oct 10, 2016 •

edited

Loading

coveralls commented Oct 10, 2016 •

edited

Loading

coveralls commented Oct 10, 2016 •

edited

Loading

coveralls commented Oct 10, 2016 •

edited

Loading

psi29a commented Oct 10, 2016 •

edited

Loading