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

Added a workaround for Python 3.5 exception on capture eventloop clos… #351

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kcexn
Copy link
Contributor

@kcexn kcexn commented Jul 6, 2019

…ing. Also added a context manager to the FileCapture class so that

managing the asyncio eventloop is easier than explicitly calling close at the end of every script. This commit fixes issue #347

I think probably there needs to be a test for the context manager so that opening and closing of files is tested properly. But I'm not sure where you'd like that test to be integrated into? Maybe a new file should be written or a new test class for FileCapture?

kcexn added 3 commits July 6, 2019 12:26
…ing. Also added a context manager to the FileCapture class so that

managing the asyncio eventloop is easier than explicitly calling close at the end of every script. This commit fixes issue KimiNewt#347
@conor-f
Copy link

conor-f commented Jul 17, 2019

I just opened issue #353 referencing this issue but related to LiveCapture. Could this be extended to fix that?

@KimiNewt
Copy link
Owner

I'm not sure if this is a good because because basically you're closing the eventloop that may be used for other things in the users' program is problematic. Especially since you might use other FileCaptures later (or you this object might be garbage-collected randomly if you're using it in a local place).

What do you think?

@kcexn
Copy link
Contributor Author

kcexn commented Aug 3, 2019

The FileCapture object shouldn't be garbage collected until there are no more references to it. So I'm reasonably confident that the risk of the FileCapture object being randomly garbage collected while in use is close to zero.

OK. Yeah. I can understand, you're saying that maybe the user already has some existing eventloop that they want to pass into the Capture object so that the new Captures can be registered in the eventloop as well, in which case you're right that this will destroy the eventloop that the user has been managing. Also it seems with newer version of python the general advice is to keep it to one eventloop per OS thread.

Maybe it might be time to consider deprecating 3.5? Otherwise I'll take a deeper look into how the async methods have been written and see if there's another way to make this work.

@KimiNewt
Copy link
Owner

KimiNewt commented Aug 3, 2019

Afraid it's a bit early to deprecate 3.5 (it's the version my workplace uses, for instance..).

I'm saying that maybe someone is making a program that uses the eventloop in various ways, making multiple FileCapture objects-- and when those will be deleted it'll screw up their flow.

The async parts definitely need refactoring (they are basically just converted trollius/python2.7 code). If you have time to check it out I can help. I don't think using 3.5 really matters, you can use async_generators if you feel you need it.

@kcexn
Copy link
Contributor Author

kcexn commented Aug 4, 2019

yeah. I get it. I'm just saying that right now each FileCapture object creates its own eventloop unless you explicitly pass in an eventloop. Even though this is possible its considered 'wrong' since 3.7 at least from what I read elsewhere. Each OS thread should run only one eventloop. Which means that FileCapture probably should check for any active event loops only create a new eventloop if one doesn't already exist. I'm far from an asyncio master so trying to refactor that will take time.

@XChikuX
Copy link

XChikuX commented Jan 31, 2023

@KimiNewt Please deprecate requests for all python versions below 3.7 and close this issue.
The industry has moved away from these versions. And all new packages on pypi are starting to adopt this trend.

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.

4 participants