Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-Implementation + Provisional dataloader #23

Merged
merged 68 commits into from
Mar 13, 2017
Merged

Conversation

syrusakbary
Copy link
Owner

@syrusakbary syrusakbary commented Feb 17, 2017

This branch updates promises with async and ultra-performant resolution and including a provisional dataloader.

Things to do before merging:

  • Have all the tests running successfully
  • Port Promise.all
  • Add promise error tracking
  • Improve Dataloader testing
  • Create a list of breaking changes
  • Investigate into Cython performance
  • Update Readme's with the new API usage

@syrusakbary syrusakbary changed the title Async dataloader Re-Implementation + Provisional dataloader Mar 13, 2017
@syrusakbary syrusakbary merged commit 791a16b into master Mar 13, 2017
@LivInTheLookingGlass
Copy link
Contributor

Pulled master after this merge and it looks like promise creation is still calling things synchronously. Am I missing something here?

@syrusakbary
Copy link
Owner Author

syrusakbary commented Mar 17, 2017

Hi @gappleto97!

I just added a feature so you can change the scheduler quite easily:

from promise import set_default_scheduler, ThreadScheduler

set_default_scheduler(ThreadScheduler())

Hope this helps! :)

@LivInTheLookingGlass
Copy link
Contributor

Unfortunately not. It looks like the problem is stemming from promise.Promise._resolve_from_executor()

That method appears to ignore the scheduler logic entirely, and just make a direct call.

@syrusakbary
Copy link
Owner Author

syrusakbary commented Mar 17, 2017

Hi @gappleto97, that is the desired behavior.

The Promise executor should always be called in the same thread where you are creating the promise, is the then (onFulfilled, onRejected) method the one that should be resolved off the thread.

This is also reflected in the spec: https://promisesaplus.com/#point-34 https://promisesaplus.com/#point-67

For clarifying, here are some JS examples that demonstrate this behavior:

promise = new Promise(function (resolve, reject) {
    // This is executed immediately
    console.log("RESOLVED"));
    resolve(true);
})
console.log("AFTER PROMISE")

(this should output "RESOLVED" and then "AFTER PROMISE")

However, is the then execution is the one that is asynchronous:

p = new Promise(function (resolve, reject) {
    // This is executed immediately
    console.log("RESOLVED");
    resolve(true);
}).then(function(resolved) {
    // This is executed in a new thread after the call stack is empty
    console.log("CALLED THEN", resolved);
});
console.log("AFTER PROMISE");

(this should output ("RESOLVED", "AFTER PROMISE", "CALLED THEN true")).

@syrusakbary
Copy link
Owner Author

Some other projects, for achieve an "async" promise execution (not resolution), use the trick of creating a resolved promise and deferring the execution on the promise resolution:

resolvedPromise = Promise.resolve();
resolvedPromise.then(function() {
// This is called async
});

https://github.com/facebook/dataloader/blob/master/src/index.js#L210
https://github.com/apollographql/graphql-server/blob/master/packages/graphql-server-core/src/runQuery.ts#L55

@LivInTheLookingGlass
Copy link
Contributor

Huh. That's surprising to me. I guess I never noticed because you can't sleep in JavaScript. On the other hand, now I don't know quite how to proceed.

Would you be willing to add a keyword for using the scheduler in this case? Another option would be to have a single modifier inheritor (ScheduledPromise) where that one method is replaced.

@LivInTheLookingGlass
Copy link
Contributor

A motivation to do this in that making it asynchronous in JavaScript is truly easy with a setTimeout, but there is no equivalent tool here

@LivInTheLookingGlass
Copy link
Contributor

So I guess what I'm asking for is the ability to do:

new Promise((accept, reject)=>{
    setTimeout(()=>{
        accept(1);
    }, 2000);
});

In your library this is currently not possible.

@syrusakbary
Copy link
Owner Author

syrusakbary commented Mar 21, 2017

Hi @gappleto97,

Something similar could be implemented with python promise :)

from threading import Timer
def set_timeout(fn, time):
    timer = Timer(time, fn)
    timer.start()

p = Promise(lambda accept, reject: set_timeout(lambda: accept(1), 2.0))

Does that work for your case?

@LivInTheLookingGlass
Copy link
Contributor

It works, but it adds exactly the sort of overhead you were looking to avoid with this. At that point you might as well just start a thread in the first place.

@syrusakbary
Copy link
Owner Author

syrusakbary commented Mar 21, 2017

I understand your frustration here, but I think is worth to keep the current python closer to the current Promise specifications.

I think, for your case, might be worth for you to extend the current promise implementation enabling this behavior:

from threading import Thread

class ThreadedPromise(Promise):
    def __init__(self, executor):
        def threaded_executor(resolve, reject):
            thread = Thread(target=executor, args=(resolve, reject))
            thread.start()
        super(ThreadedPromise, self).__init__(threaded_executor)

t = ThreadedPromise(lambda resolve, reject: resolve(1))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants