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

fix: make promise library threadsafe #80

Closed
wants to merge 1 commit into from

Conversation

diverru
Copy link

@diverru diverru commented Nov 24, 2019

Problem proof: https://gist.github.com/diverru/3b317313366311fc260707cc91e77994
relating problems: #57, #68, #70
My solution is more lightweight than #70

Also, i've found that graphql-core uses promises to handle whole graphql requests, it means that even with SyncExecutor whole graphql request could be handled by another unexpected thread.
It leads to huge security problems with global/singleton (even threadlocal) variables.

@diverru diverru changed the title fix: make promise library threadsafe fix: make promise library threadsafe again Nov 25, 2019
@diverru diverru changed the title fix: make promise library threadsafe again fix: make promise library threadsafe Nov 25, 2019
@jnak jnak mentioned this pull request Dec 6, 2019
@jnak
Copy link
Contributor

jnak commented Dec 14, 2019

@diverru I just submitted another PR that fixes the thread-safety issues in both Promise and Datalaoder. It includes 2 detailed tests / reproductions. Would you mind taking a look at it? It is critical we get this expedited. Thanks

@diverru
Copy link
Author

diverru commented Dec 15, 2019

@jnak recently author approved your PR and it's better then mine

@syrusakbary
Copy link
Owner

Closing this in favor of already merged #81

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