syrusakbary / promise

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

KeyError in DataLoader leads to a stuck event loop #29

Closed rslinckx closed 7 years ago

rslinckx commented 7 years ago
from promise.dataloader import DataLoader

def batch_load_foo(keys):
    return 23

loader = DataLoader(batch_load_foo)
loader.load(1)
loader.load(2)

This will attempt to dispatch key=1, will correctly queue in the event loop the batch loader, which will then receive the incorrect return value of '23' from the batch_load_fn.

It will then call failed_dispatch with the error, which will attempt to reject all pending promises, and also call loader.clear(1). The problem is:

def clear(self, key):
    cache_key = self.get_cache_key(key)
    del self._promise_cache[cache_key]
    return self

this will throw a KeyError because the promise hasn't been cached yet, and will stall the event loop. Worse, the exception is catched by the SyncScheduler and things just stop.

A fix would be to self._promise_cache.pop(cache_key, None) for example, but i'm not sure if it has deeper implications...

rslinckx commented 7 years ago

Also is the try/catch on SyncScheduler really necessary ?

syrusakbary commented 7 years ago

Your example should fix the issue :) (PR? :) )

PS: yeah, the try/catch is necessary safe/guard as, if there is any issue in the function resolution, the async instance might block any subsequent calls.

rslinckx commented 7 years ago

You can review pull request #31