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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: If running in Windows, do not use add_signal_handler() #274

Merged

Conversation

joshua-janicas
Copy link
Contributor

@joshua-janicas joshua-janicas commented Jun 5, 2024

What is the issue?

image

What was changed?

  • At first I was trying to figure out how to implement the fix for windows: signal.raise_signal(signal.SIGINT) via https://stackoverflow.com/questions/35772001/how-to-handle-a-signal-sigint-on-a-windows-os-machine/72637975#72637975. The trick is that we are dealing with sub-processes here. My Python knowledge is limited at this time since Windows can't use add_signal_handler - I wasn't quite sure how to best go this way to have the subprocess use it.
  • Another possible option instead is to not invoke the add_signal_handler during local Windows development - which is what this PR is trying to do.

Example

  1. Attempting to run development in Windows with loop.add_signal_handler
    image

  2. == to != for win32 check
    image

  3. Checking KeyboardInterrupt works
    image

Other notes

We may also be able to set an event loop policy around if the platform we are running on is in Windows - before we get the event loop afterwards. However like I mentioned above my Python knowledge is limited, especially with how async/event loops are managed. I don't think it's a good idea to introduce it but I did want to mention I took a look anyways.

        # If running in windows, set event loop policy instead
        if sys.platform == 'win32':
            asyncio.set_event_loop_policy(asyncio.WindowsProactorEventLoopPolicy())
            loop = asyncio.ProactorEventLoop()
            asyncio.set_event_loop(loop)

馃摎 Documentation preview 馃摎: https://meltano-edk--274.org.readthedocs.build/en/274/

@joshua-janicas joshua-janicas changed the title Fix: If platform is Windows, do not use add_signal_handler() fix: If platform is Windows, do not use add_signal_handler() Jun 5, 2024
@joshua-janicas joshua-janicas changed the title fix: If platform is Windows, do not use add_signal_handler() fix: If developing in Windows, do not use add_signal_handler() Jun 5, 2024
@joshua-janicas joshua-janicas changed the title fix: If developing in Windows, do not use add_signal_handler() fix: If running in Windows, do not use add_signal_handler() Jun 5, 2024
Copy link
Member

@WillDaSilva WillDaSilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this thorough investigation and fix @joshua-janicas!

@WillDaSilva WillDaSilva merged commit c5dc08d into meltano:main Jun 6, 2024
10 of 11 checks passed
@WillDaSilva
Copy link
Member

@joshua-janicas This fix has been published in https://github.com/meltano/edk/releases/tag/v0.4.2

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.

None yet

2 participants