search

syrusakbary / promise

Ultra-performant Promise implementation in Python
MIT License
362 stars 76 forks source link

Hanging dataloader #41

Open chaffeqa opened 7 years ago

chaffeqa commented 7 years ago

We have experianced issues with v2.0.1 where a promise(s) would not be resolved, and the graphene request would hang forever. I wanted to get a totally hopeful response that this may be a known issue, and that it is solved in https://github.com/syrusakbary/promise/commit/cb9bbff1e85bead94c1b10fc36ce95d5d0fc3ce3

syrusakbary commented 7 years ago

Hi @chaffeqa, Thanks for the response, that's something that happened before as you pointed, and should be easy to fix.

Could be possible to have a quick overview of your installed packages (version of graphene, promises, graphene-*, ...). A github repo or a code sample that could help us to reproduce the issue will be also helpful!

chaffeqa commented 7 years ago 
# graphql tool
##############################
# graphene
# - Current version: https://github.com/graphql-python/graphene/commit/078230ad4906465a0757df7ca490c2990421d7d6
# - See missing updates: https://github.com/graphql-python/graphene/compare/078230ad4906465a0757df7ca490c2990421d7d6...graphql-python:master
graphene==1.4.1
# graphene-django
# - Current version: https://github.com/graphql-python/graphene-django/commit/cec1a8448048ce55a2844df63b295b8d9d5000d7
# - See missing updates: https://github.com/graphql-python/graphene-django/compare/cec1a8448048ce55a2844df63b295b8d9d5000d7...graphql-python:master
git+https://github.com/graphql-python/graphene-django.git@cec1a8448048ce55a2844df63b295b8d9d5000d7#egg=graphene-django
# - Current version: https://github.com/graphql-python/graphql-core/commit/bf9e156b791f349ab91f113e3dee0731331ad59f
# - See missing updates: https://github.com/graphql-python/graphql-core/compare/bf9e156b791f349ab91f113e3dee0731331ad59f...graphql-python:master
git+https://github.com/graphql-python/graphql-core.git@bf9e156b791f349ab91f113e3dee0731331ad59f#egg=graphql-core

Unfortunately I don't have a good bit of sample code, the integration was pretty intense though, lots of records, large batches, but I'm fairly confident that for every key we would try to load, we would return either a record or None in the correct array position

syrusakbary commented 7 years ago

Version of promise?

chaffeqa commented 7 years ago

it seemed to resolve to 2.0.1

Requirement already satisfied: promise>=2.0 in /usr/local/var/pyenv/.../src/promise (from graphene==1.4.1->-r requirements/../requirements.txt (line 60))

# fresh install:
Collecting promise>=2.0 (from graphene==1.4.1->-r requirements/../requirements.txt (line 60))
  Using cached promise-2.0.2.tar.gz
syrusakbary commented 7 years ago

This issue is only caused by the promises package.

I believe the issue was fixed in 2.0.2 with this commit: https://github.com/syrusakbary/promise/commit/57d205889787199d2e6e1ea91e76da41cf6b1fc4.

You can try to see if with 2.0.2 this issue is still happening. If that's the case, might be worth to try: pip install "promise>=2.1.dev".

Please keep me in the loop with the results :)

chaffeqa commented 7 years ago

Actually we ran into that issue, but only with the clear(key).prime(key, record) usage.

However that doesn't seem to address the issue of "hanging requests" due to unfulfilled promises (unless I'm missing something deeper in the code that it effects)

The main reason for writing this is because I'm curious whether this commit may deal with the bug we were having. I didn't dive in deep enough to see what case it addresses, but we definitely didnt have it in our codebase

chaffeqa commented 7 years ago

Thanks for the info! We can confirm 2.0.2 fixes the issue we saw.

I have a further question though, I did some diffing to see the changes (FYI I think the git tags and pypi versions are pretty confusing right now, but understandable since you have so many projects to keep in sync here 😃 ):

I wanted to ask about how you feel 2.1.x (master) changes from 2.0.2 are as:

chaffeqa commented 7 years ago

I just sent you an email containing a stacktrace referencing a bug we encountered on 2.0.2, we are trying now on 2.1.dev20170724043809and will update if we see it there too. We are hoping that the change cleans up the bug.

From my understanding, the callback chain is losing context somewhere, and ends up calling context_instance._exit() inside a callback, but that function has lost its context, and self._exited throws a AttributeError since it doesn't exist (or has been GC'd?).

Either way, I wish I had more time to isolate the issue and make a test, but at this point this is the best I can do. Hope its helpful, and even more that the issue is fixed with your moving away from Context 🎉