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

Python 3.6 does not support asyncio.get_event_loop().add_signal_handler and async_client.py spams about it on every connect attempt #276

Closed
bialix opened this issue May 24, 2022 · 8 comments
Assignees
Labels

Comments

@bialix
Copy link

bialix commented May 24, 2022

engineio/asyncio_client.py has the following code at the very start of connect:

        global async_signal_handler_set
        if self.handle_sigint and not async_signal_handler_set and \
                threading.current_thread() == threading.main_thread():
            try:
                asyncio.get_event_loop().add_signal_handler(
                    signal.SIGINT, async_signal_handler)
                async_signal_handler_set = True
            except NotImplementedError:  # pragma: no cover
                self.logger.warning('Signal handler is unsupported')

The problem here is that variable async_signal_handler_set will never be set to True in this case and logger is spamming my logs with useless warning.

May I suggest to rename async_signal_handler_set to async_signal_handler_once and set it regardless of exception raised to avoid it triggered next time? I can make PR if you agree.

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented May 24, 2022

Yes, your suggestion makes sense. Thanks. If you create the PR I'll review and merge.

@miguelgrinberg miguelgrinberg self-assigned this May 24, 2022
@bialix
Copy link
Author

bialix commented May 24, 2022

The same problem with Python 3.10 @ Windows and I'm not sure why. Maybe it's a asyncio bug on Windows.

@miguelgrinberg
Copy link
Owner

@bialix I believe there is no support for signal handlers in the Windows event loop, so there is currently no way to exit cleanly on that platform.

@bialix
Copy link
Author

bialix commented May 25, 2022

Yep, I think you're right.

So, should I compose a patch or you do it yourself?

@bialix
Copy link
Author

bialix commented May 25, 2022

OK, that's definitely problem with your signal_handler.
Once you set it - your handler overrides my own SIGINT handler. And I'm not sure it's desired side-effect.

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented May 25, 2022

I'm going to close this issue, since you have now moved the discussion to #277.

@bialix
Copy link
Author

bialix commented May 25, 2022

#277 is about disconnect method. This issue is about spaming with warnings. I don't understand, but it's up to you to close issues, that's your project.

@miguelgrinberg
Copy link
Owner

I settled the discussion on the multiple attempts to set a signal in the 2nd message in this issue:

Yes, your suggestion makes sense. Thanks. If you create the PR I'll review and merge.

Everything else in this issue is unrelated to that problem. In any case, I just submitted the fix for the original issue you reported here. Now the signal will be set the first time you connect only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants