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

AsyncIOEventEmitter should keep a reference to scheduled futures #120

Closed
ofacklam opened this issue Mar 10, 2023 · 5 comments · Fixed by #123
Closed

AsyncIOEventEmitter should keep a reference to scheduled futures #120

ofacklam opened this issue Mar 10, 2023 · 5 comments · Fixed by #123

Comments

@ofacklam
Copy link

The documentation for asyncio.ensure_future indicates:

Important See also the create_task() function which is the preferred way for creating new Tasks.
Save a reference to the result of this function, to avoid a task disappearing mid-execution.

Currently, this doesn't seem to be the case in AsyncIOEventEmitter, which to my understanding only adds a "done" callback, but doesn't keep the future's reference any longer.

@jfhbrook
Copy link
Owner

I think that assigning to fut and having callback close around it might be enough. I really don't want to get in the business of storing futures in a set and tests are consistently behaving so I'm going to hold off on anything more obvious, but if there ends up being a concrete bug I'm happy to take a closer look.

@auxsvr
Copy link

auxsvr commented May 1, 2023

When _emit_run finishes, fut is out of scope, so the future may be garbage-collected. This causes a bug in playwright that loses events, as the corresponding futures vanish before they're fully executed and the corresponding error message appears when the program finishes. A solution is to store the futures in a file-level variable, then use the callback to remove them once finished.

@hidaris
Copy link

hidaris commented Jun 7, 2023

Is there any further conclusion here?

@jfhbrook
Copy link
Owner

jfhbrook commented Jun 9, 2023

I'm currently pushing a fix in v9.1.1 - give it a shot, and if you still have issues let me know.

@elacuesta
Copy link

For the record, we're still observing this issue with pyee==11.0.0 (see scrapy-plugins/scrapy-playwright#188 & scrapy-plugins/scrapy-playwright#233).
I'm mostly leaving this message to keep track of this, at the time I'm not able to provide a reproducible example using only pyee and I'm well aware that the issues I mentioned add two layers of dependencies 😞.

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 a pull request may close this issue.

5 participants