Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

Deferred resolving doesn't seem to work #336

Closed
Jalle19 opened this issue Nov 10, 2018 · 15 comments
Closed

Deferred resolving doesn't seem to work #336

Jalle19 opened this issue Nov 10, 2018 · 15 comments
Assignees
Labels
bug Something isn't working execution Related to execution performance Related to perfomance

Comments

@Jalle19
Copy link
Contributor

Jalle19 commented Nov 10, 2018

There's something wrong with it. In the test we end up calling loadBuffered() twice, which shouldn't happen. There's also both $directorIds and $authors, which seems wrong. Gotta look this through carefully.

@Jalle19 Jalle19 added bug Something isn't working execution Related to execution performance Related to perfomance labels Nov 10, 2018
@Jalle19
Copy link
Contributor Author

Jalle19 commented Nov 10, 2018

Related to #320

@crisu83
Copy link
Contributor

crisu83 commented Nov 12, 2018

@Jalle19 I took a look at this and you're right. The issue is that the promise is resolved immediately. I need to do some research. It might be that we need to introduce our own Deferred concept, like they have done in Webonyx implementation. I don't like the idea, but if it that's what it takes then we'll do that.

@inanc-g
Copy link

inanc-g commented Mar 18, 2019

Any news on this?

@Jalle19
Copy link
Contributor Author

Jalle19 commented Mar 18, 2019

@crisu83 do you have time to look into this?

@Jalle19
Copy link
Contributor Author

Jalle19 commented Mar 18, 2019

or @hungneox

@acelot
Copy link
Contributor

acelot commented Mar 21, 2019

Confirm this issue. loadBuffered() is called everytime.

@acelot
Copy link
Contributor

acelot commented Mar 25, 2019

Guys, this issue is critical for production use. Do you have any suggestions? If you do not have time, tell me which way to dig?

@asheliahut
Copy link

This is literally the only reason I haven't swapped to this over my Youshido Fork.

@crisu83
Copy link
Contributor

crisu83 commented Apr 3, 2019

@acelot I have my hands full right now with work, but the easiest way to look at it is to debug the deferred resolver test cases. I think that the main issue is related to how we're handling promises when executing them. CC @hungneox

@asheliahut
Copy link

Is there any thought to use https://github.com/overblog/dataloader-php
to solve the problem like almost everyone in node does?

@acelot
Copy link
Contributor

acelot commented May 17, 2019

I'm trying to adapt this library for my reactphp-project. I was able to perform a deferred request using lordthorzonus/php-dataloader. But for this I had to change the graphql-php code to support async calls. Because the existing executor have a race condition problem Execution::execute():

if ($data instanceof PromiseInterface) {
    $data->then(function ($resolvedData) use (&$data) {
        $data = $resolvedData;
    });
}

<...>

return new ExecutionResult($data, $context->getErrors());

In this section of the code, the $data variable will always be null and will throw an exception.
To add support for asynchronous calls, I suggest adding a new method promiseExecute which implements new PromisedExecutionInterface:

class Execution implements ExecutionInterface, PromisedExecutionInterface
{
    <...>

    public function promiseExecute(
        Schema $schema,
        DocumentNode $documentNode,
        $rootValue = null,
        $contextValue = null,
        array $variableValues = [],
        ?string $operationName = null,
        ?callable $fieldResolver = null,
        ?ErrorHandlerInterface $errorHandler = null
    ): PromiseInterface {
        <...>

        if ($data instanceof PromiseInterface) {
            return $data->then(function ($resolvedData) use ($context) {
                return new ExecutionResult($resolvedData, $context->getErrors());
            });
        }

        <...>

        return resolve(new ExecutionResult($data, $context->getErrors()));
    }

    <...>
}

I can provide PR, but I need help with tests.

@Jalle19
Copy link
Contributor Author

Jalle19 commented May 17, 2019

@acelot PRs are more than welcome, I'll help you out with the tests!

@acelot
Copy link
Contributor

acelot commented May 18, 2019

@Jalle19 #345

@crisu83
Copy link
Contributor

crisu83 commented Jul 30, 2019

@acelot is this issue resolved?

@acelot
Copy link
Contributor

acelot commented Jul 30, 2019

Yes, completely.

@crisu83 crisu83 closed this as completed Nov 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working execution Related to execution performance Related to perfomance
Projects
None yet
Development

No branches or pull requests

6 participants