Start resolver in new thread, to replicate node.js Promise behavior

LivInTheLookingGlass commented 7 years ago

This patch also makes some other minor changes:

Python 2 default timeout is now float("Inf"), rather than None, so that it still listens to KeyboardInterrupt
API doc is updated to reflect the code running asynchronously
Imports were trimmed to grab only what's used
String formatting now uses "".format()
One test imports a stdlib function, rather than redefining it, as previously
Test added for #9

From my testing, this should also fix #9

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling 0ac2b36f1a1dfc9116b9ccd0dd67c64d44eefa44 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling 0ac2b36f1a1dfc9116b9ccd0dd67c64d44eefa44 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling a48f549269f586ff8649673f5f51404a55f8e6a8 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling a48f549269f586ff8649673f5f51404a55f8e6a8 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling a48f549269f586ff8649673f5f51404a55f8e6a8 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage decreased (-3.0%) to 97.046% when pulling e19a6afc0aa927470d7f85c7ca7788c64b76f8a9 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling c4ae52bd54b09478869b1e0c9e9c748aa1372b54 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling c4ae52bd54b09478869b1e0c9e9c748aa1372b54 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling c4ae52bd54b09478869b1e0c9e9c748aa1372b54 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling ca4b8892b3e3519717e5e0e6bfe25524d71a5414 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling ca4b8892b3e3519717e5e0e6bfe25524d71a5414 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling ca4b8892b3e3519717e5e0e6bfe25524d71a5414 on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling 53513019f59a88ab381967fac434814c67115ebf on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling 53513019f59a88ab381967fac434814c67115ebf on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling 53513019f59a88ab381967fac434814c67115ebf on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling d5743ef2be5b648d0d1da5924f95a4718fc2eedc on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling d5743ef2be5b648d0d1da5924f95a4718fc2eedc on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling d5743ef2be5b648d0d1da5924f95a4718fc2eedc on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

coveralls commented 7 years ago

Coverage remained the same at 100.0% when pulling d5743ef2be5b648d0d1da5924f95a4718fc2eedc on p2p-today:master into b4ec8f19546947b97583d07f460b74e81a9eb671 on syrusakbary:master.

LivInTheLookingGlass commented 7 years ago

If you'd like, @syrusakbary, I could revise this to only include the test for #9 and the initial 4 line commit. I would understand if this is a bit large to review.

LivInTheLookingGlass commented 7 years ago

@syrusakbary, I would very much appreciate a response. I would like to use this as a dependency in my project, but I need this patch to be released before I can do anything. I cannot release my code until I have some sort of answer.

syrusakbary commented 7 years ago

Hi @gappleto97, sorry for taking long in reviewing... I've been super occupied lately.

I really like the direction of this PR as it focus on fixing #9, however I'm worried about the implications of starting a Thread for each resolved promise. In a "synchronous" python environment starting a thread might be the path to go, however in environments where gevent or asyncio (event loops) are used, starting a Thread is not really necessary and introduces a significant overhead.

I would love to see if we can find a way to make both strategies to work :)

LivInTheLookingGlass commented 7 years ago

I get where you're coming from.

There are two reasons I didn't go that route. First, because I'm mainly looking for compatibility. But more importantly, I'm just not as familiar with that side of things yet.

If you can suggest some things to investigate, I'm more than happy to do so. It may take some time to reach that level though. I would prefer if we merged the current code, with the understanding that the next version will have something more performant.

And like I said, I'm more than happy to contribute upstream.

syrusakbary commented 7 years ago

After thinking a while, I think the best strategy is create different loop strategies in this package.

I will work in a branch based on your work in this PR :)

LivInTheLookingGlass commented 7 years ago

Okay. Feel free to pull me in if you'd like the help.

syrusakbary commented 7 years ago

This is merged, but excluding the thread creation per promise (I liked your code & syntax improvements).

I'm working on an optimal schedulers approach.

LivInTheLookingGlass commented 7 years ago

Sweet, thanks! Please let me know when that is finished, or how I can help.

LivInTheLookingGlass commented 7 years ago

Shouldn't we still have the "start new thread" behavior, so that systems without asyncio can still function correctly? I admit I don't know, but wouldn't that close off 2.7 support for good?

syrusakbary commented 7 years ago

Yeah, some cases (like in Python 2.7) Threads might still be necessary.

In this case, there will be an automatic get_default_scheduler that will return the best scheduler for the environment:

Python 2.7 to 3.3: Thread (+ThreadPoolExecutor ?) scheduler
Python 3.4+: AsyncIO scheduler (using the event loop)
Other special cases: use a sync scheduler

Also, there will be a setter set_default_scheduler, that will permit developer to use custom ones (probably also provided by this library):

Gevent scheduler
Tornado scheduler
...

LivInTheLookingGlass commented 7 years ago

Okay. That'd be cool.

In the interest of being able to release, I think I'm going to stick with my fork until this is merged. However, I think what you're doing is way better, and if there's any way to help, please let me know.

syrusakbary commented 7 years ago

Will do! Thanks!!

If you have any idea of how to optimize the current code, or any other suggestions about scheduling/async please let me know.

I'm researching a lot into different promises implementations:

Bluebird (js - quite performant): https://github.com/petkaantonov/bluebird/
rsvp.js (js, also performant): https://github.com/tildeio/rsvp.js/
Promise (Swift library, quite simple implementation): https://github.com/khanlou/Promise
Promises (PHP library): https://github.com/guzzle/promises

In general, most of them use a Queue + async function invocation. The async invocations are mainly triggered on the then method in a instantiated Promise, instead of the resolution per se.

LivInTheLookingGlass commented 7 years ago

I'll do what I can to work on the current codebase. I'm not very familiar with Python's event loop setup, and I'm pretty sure I'd only slow you down on that front.

I'll definitely make sure to look into it once the design framework is put in place though.

syrusakbary / promise

Start resolver in new thread, to replicate node.js Promise behavior #20