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

import side-effect disables promise trampoline breaking DataLoaders #67

Open
AllexVeldman opened this issue Mar 19, 2021 · 7 comments
Open

Comments

@AllexVeldman
Copy link

AllexVeldman commented Mar 19, 2021

On import channels_graphql_ws, promise.promise.async_instance.disable_trampoline() is called, breaking DataLoaders.

In the code there is a reference to syrusakbary/promise#57 (comment) which indicates this is needed due to Promise not being thread-safe.

This should no longer be needed with promise ^2.3
syrusakbary/promise#81
https://github.com/syrusakbary/promise/releases/tag/v2.3.0

For some reason not completely clear to me yet this only impacts my unittests, the async_instance during normal operation seems to have trampoline_enabled=True.

@prokher
Copy link
Member

prokher commented Mar 19, 2021

@alexmnazarenko Considering this thread-safe issue as fixed we can probably throw away invocation of disable_trampoline(). We need to be careful here, AFAIR it took significant about of time to debug this for the first time.

@matclayton
Copy link

We're seeing the exact same issue, enable_trampoline() fixed it

@tony
Copy link

tony commented Feb 1, 2022

@matclayton

We're seeing the exact same issue, enable_trampoline() fixed it

Any example of where you (or anyone else who happens upon this issue) placed the enable_trampoline() code?

(I assume if @AllexVeldman is correct on promise not needing it, doing it as early as possible would be best)

@matclayton
Copy link

For sure

class GraphQLConfig(AppConfig):
    name = "mixcloud.core.graphql"
    label = "graphql"

    def ready(self) -> None:
        # This is dangerous
        # This is to counteract https://github.com/datadvance/DjangoChannelsGraphqlWs/blob/master/channels_graphql_ws/graphql_ws_consumer.py#L165
        # DjangoChannelsGraphqlWs disables_trampoline, which by default prevents dataloaders and promises from working, t
        # This would cause a LOT of tests to fail,
        # We need to put this back to the original value after DjangoChannelsGraphqlWs has done the damage.
        # These is a plan to remove this from the base lib https://github.com/datadvance/DjangoChannelsGraphqlWs/issues/67
        # as promises 2.4+ is now thread safe, so their patch shouldn't be required anymore, enable_tramoline could be removed at that point.
        import promise

        promise.promise.async_instance.enable_trampoline()

We do this in an AppConfig, so its enabled at bootup

@tony
Copy link

tony commented Feb 2, 2022

@matclayton Thank you for this!

@malthejorgensen
Copy link

malthejorgensen commented Mar 8, 2022

For what it's worth, our (@tony and I) issue was this: graphql-python/graphql-core-legacy#287

@adrienhongcs
Copy link

Are there any plans to remove promise.promise.async_instance.disable_trampoline() in the next release?

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

No branches or pull requests

6 participants