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

Handle CTRL+BREAK signals hackrf-tools on Windows #1493

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scootermon
Copy link

When running hackrf_transfer as a subprocess under Windows we've found that it's exceptionally hard to stop the process.
It turns out to be much easier to send a CTRL_BREAK_EVENT signal to a process than a CTRL_C_EVENT.

@martinling
Copy link
Member

The change looks fine to me, but I'm curious why it's easier to send one than the other.

@scootermon
Copy link
Author

scootermon commented Oct 18, 2024

To be honest that is a bit of mystery to me as well. I'm very much a linux person, so I'm not too familiar with Windows' process model.
When I send a CTRL_C_EVENT to the process it simply appears to never arrive.
Some quick googling revealed that signals are a way bigger issue on Windows than I expected. I took a look at the underlying function used by Windows GenerateConsoleCtrlEvent and noticed there's some remarks about how CTRL+C might end up not triggering the handler whereas CTRL+BREAK always does. Now again, full transparency, my understanding of Windows isn't good enough to really understand what exactly is going on here, but once I added the handling for the BREAK signal it just worked.

Looking around I saw a lot of projects treat these signals as interchangeable. See for instance the go runtime: https://github.com/golang/go/blob/6853d89477e0886c7c96b08e7efaf74abedfcf71/src/runtime/os_windows.go#L1102-L1103
So even if I'm simply doing something wrong and it could also work with CTRL+C, I still think this change isn't completely unjustified.

For reference, here's some Python code I used for testing:

import asyncio
import signal
import subprocess


async def main():
    proc = await asyncio.create_subprocess_exec(
        "hackrf_transfer.exe", "-t", "data.bin",
        creationflags=subprocess.CREATE_NEW_PROCESS_GROUP,
    )
    await asyncio.sleep(1)
    proc.send_signal(signal.CTRL_C_EVENT)
    # this will hang forever. Using `CTRL_C_BREAK` (with the changes from this PR) allows it to work
    await proc.wait()


asyncio.run(main())

Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification.

I wondered if the unreliability might be something about the different threads involved, and which one ended up getting the event - we had some issues before with signal masks on worker threads spawned by libhackrf.

But from some reading it seems that on Windows, a console event causes a new thread to be started to handle that event, rather than the Unix model where the signal is received by an existing thread.

In any case, it seems perfectly reasonable that we should stop on a Ctrl-Break event in the same way as we do for a Ctrl-C event, so I'm fine with making this change.

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.

2 participants