Fix critical unhandled error

[lucagiovagnoli]

After having had an interest read from Twisted Programming Essentials I got an idea about what could be the cause for issue #27.

Possible causes

1. First, the way we are catching exceptions might be problematic. Here we should at least return the failure or raise the exception, but we are only setting the future's error. The book states:

   If a callback or errback at level N doesn’t raise an Exception or return a Failure, control is passed to the callback at level N + 1. Note that this applies to errbacks! If an errback doesn’t produce an error, control passes to the callback chain. Control will criss-cross between the errback and callback chains depending on the results of processing the event."

2. Secondly, this is stated as a "please don't do" in crochet documentation as Twisted APIs are not thread safe and should not be called if not in the Twisted reactor thread. I believe we should put back the @run_in_reactor decorator at all costs.

   Twisted’s APIs are not thread-safe, and so they cannot be called directly from another thread. Moreover, results may not be available immediately. The easiest way to deal with these issues is to decorate a function that calls Twisted APIs with crochet.wait_for. [...] wait_for is implemented using run_in_reactor, a more sophisticated and lower-level API.

Solution

I am proposing here a way simpler solution (literally by the book, chapter "Web Clients - Agent") which solves the issue of CRITICAL errors #27 and seems to work properly with our internal system. The general idea is that there is no need for futures as the EventualResult object from crochet already acts as some sort of future. Hence there's no need to set results/exceptions in the future ourselves, as the EventualResult does it already flawlessly and without risk of losing 'unhandled exceptions' as it was happening.

Testing

Finally, I tested this client with our services and our acceptance suite was all green.

[coveralls]

Coverage decreased (-1.4%) to 97.5% when pulling f1ea476 on lucagiovagnoli:fix-critical-unhandled-error into 1b30e31 on Yelp:master.

[NicolasEhrhardt]

key.lower() != "content-length" is more readable.

[lucagiovagnoli]

oops, yep, I had just introduced the .lower() in PR #26.
This is a merge with a past branch of mine ended poorly.

Thanks for taking the time to review Nicolas!

[coveralls]

Coverage increased (+0.05%) to 98.947% when pulling 68bdfa3 on lucagiovagnoli:fix-critical-unhandled-error into 1b30e31 on Yelp:master.

[coveralls]

Coverage increased (+0.05%) to 98.947% when pulling bc8051b on lucagiovagnoli:fix-critical-unhandled-error into 1b30e31 on Yelp:master.

[laucia]

importing reactor is something we know can lead to problems because it leads to itamarst/crochet#45 like issues

[askedrelic]

+1 for style and docs. The why behind the change is rough to understand, but I think we will be in a better place with this.

[coveralls]

Coverage increased (+0.05%) to 99.0% when pulling 907a11c on lucagiovagnoli:fix-critical-unhandled-error into 8273a69 on Yelp:master.

[lucagiovagnoli]

merged with #29, fixed issues, improved tests and documentation. acceptance tested with our internal ecosystem.

[askedrelic]

How about "Sane defaults to prevent infinite timeouts"

[lucagiovagnoli]

missed this, done now

[laucia]

lg2m

[coveralls]

Coverage increased (+0.05%) to 99.0% when pulling b324768 on lucagiovagnoli:fix-critical-unhandled-error into 8273a69 on Yelp:master.

[askedrelic]

🚢

[laucia]

       _~
    _~ )_)_~
    )_))_))_)
    _!__!__!_
    \______t/

[lucagiovagnoli]

amended - just fixing comments and typos

[coveralls]

Coverage increased (+0.05%) to 99.0% when pulling 24765d4 on lucagiovagnoli:fix-critical-unhandled-error into 8273a69 on Yelp:master.

[prat0318]

will go through it over the weekend. but i think @jnb and @analogue will have better context.

[coveralls]

Coverage increased (+0.09%) to 99.038% when pulling d379944 on lucagiovagnoli:fix-critical-unhandled-error into 8273a69 on Yelp:master.

[laucia]

When travis complete, I think we are good to go!

[coveralls]

Coverage increased (+0.09%) to 99.038% when pulling 9b274a4 on lucagiovagnoli:fix-critical-unhandled-error into 8273a69 on Yelp:master.