Update start/stop handling of credential_manager_shim

[arenekosreal]

• Use asyncio.run or uvloop.run to start main loop
• Handle SIGTERM to quit gracefully

Using asyncio.run is preferred by Python.
asyncio.get_event_loop raises a RuntimeError on my machine, which means that there is no event loop set.
This is a change in 3.14, seems that we need to use asyncio.set_event_loop before running. So I use asyncio.run directly to avoid this.

Handle SIGTERM signal so we can quit gracefully.
Firefox will send SIGTERM on *nix systems, and use Windows's way to kill the native app. Not using Windows so not sure if SIGTERM will also be raised on Windows, but I do not find a clean way to notify killing.

[kalvdans]

Stick to one framework, if we need uvloop, change the README.md file to pip install it.

[arenekosreal]

Actually uvloop is completely optional. If the script failed to import it, it will simply fallback to use the built-in asyncio. Anyway, I will update README to mention about this later.

[kalvdans]

Could you also motivate in the PR description why we need to quit gracefully instead of just terminating abruptly. Thanks.

[iinuwa]

Hello! Thanks for your contribution!

I copied in the text from your commit message into the PR description for visibility.

Firefox will send SIGTERM on *nix systems, and use Windows's way to kill the native app. Not using Windows so not sure if SIGTERM will also be raised on Windows, but I do not find a clean way to notify killing.

We only target Linux, so we don't need to worry about Windows. 👍

[iinuwa]

This is not a hot code path, so let's drop the uvloop implementation.

[iinuwa]

please import asyncio, and use asyncio.run to refer to this method.

[arenekosreal]

@kalvdans At first I was creating a package on AUR, and found that this helper cannot start because asyncio.get_event_loop had raised a RuntimeError. During fixing this problem, I noticed that this helper always runs in a while-loop and never breaks, then I changed the code to let it break the loop when needed.

[arenekosreal]

@iinuwa I have force-pushed changes, uvloop support is removed completely.

[kalvdans]

I noticed that this helper always runs in a while-loop and never breaks, then I changed the code to let it break the loop when needed.

Are you saying that asyncio is swallowing KeyboardInterrupt? Reading online I see that asyncio turns KeyboardInterrupt into CancelledError. And the base class for CancelledError was changed from Exception to BaseException in 3.8 so it shouldn't be caught by except Exception clauses.

[arenekosreal]

Are you saying that asyncio is swallowing KeyboardInterrupt?

No, just the literal meaning, I noticed a never-ending while-loop, and then I made it breakable.

[kalvdans]

No, just the literal meaning, I noticed a never-ending while-loop, and then I made it breakable.

But a KeyboardInterrupt will break that infinite loop. I tested the following code on Python 3.12:

import asyncio

while True:
    loop = asyncio.new_event_loop()
    loop.run_until_complete(asyncio.sleep(100))

and it breaks fine with CTRL-C, despite the inifinte loop:

$ python3 testme.py 
^CTraceback (most recent call last):
  File "/home/chn/tmp/testme.py", line 5, in <module>
    loop.run_until_complete(asyncio.sleep(100))
  File "/usr/lib/python3.12/asyncio/base_events.py", line 674, in run_until_complete
    self.run_forever()
  File "/usr/lib/python3.12/asyncio/base_events.py", line 641, in run_forever
    self._run_once()
  File "/usr/lib/python3.12/asyncio/base_events.py", line 1949, in _run_once
    event_list = self._selector.select(timeout)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/selectors.py", line 468, in select
    fd_event_list = self._selector.poll(timeout, max_ev)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyboardInterrupt

My wish is that we should look closer into why this mechanism is broken in credential_manager_shim instead of painting over the problem.

[arenekosreal]

and it breaks fine

All unhandled exceptions risen in the loop can break the loop and force the program exit abnormally. You will get a traceback, just like the output in your comment. If the program is exited with an unhandled exception, that does not mean fine, you can check its return code, it is not the normal 0 , it is 130.

Besides, Ctrl-C will send SIGINT instead SIGTERM according to Wikipedia, you may make a mistake here.

Sending a SIGTERM manually does not raise an exception, but also returns non-zero code:

$ python -c 'import asyncio

  while True:
      loop = asyncio.new_event_loop()
      loop.run_until_complete(asyncio.sleep(100))
  '
fish: Job 1, 'python -c 'import asyncio

while True:
    loop = asyncio.new_event_lo…
    loop.run_until_complete(asy…
'' terminated by signal SIGTERM (Polite quit request)

$ echo $status
143

My commit is going to handle this correctly:

$ python -c 'import asyncio
  import signal

  event = asyncio.Event()

  signal.signal(signal.SIGTERM, lambda _, __ : event.set())

  while not event.is_set():
      loop = asyncio.new_event_loop()
      loop.run_until_complete(asyncio.sleep(100))

  '

$ echo $status
0

[kalvdans]

and it breaks fine

All unhandled exceptions risen in the loop can break the loop and force the program exit abnormally. You will get a traceback, just like the output in your comment. If the program is exited with an unhandled exception, that does not mean fine, you can check its return code, it is not the normal 0 , it is 130.

Thanks, you are right I mixed up SIGTERM and SIGINT. But the question remains, why is the exit status inportant to Firefox? If it sends SIGTERM to kill the program, I don't think it care if the exit code is 0 or 130 or died from signal. I believe it is important for reviewers and code readers what other side-effects of a graceful exit we want, besides the exit code.

[arenekosreal]

why is the exit status inportant to Firefox?

It is because Mozilla's documentation says firefox will send SIGTERM to its extensions' native app part when the extension is closing. We need to ensure the script can exit normally when meeting SIGTERM. Or we will get SIGKILL from firefox.

I don't think it care if the exit code is 0 or 130 or died from signal.

I agree. But we have a chance to quit program gracefully instead wait for being killed now, I think we'd better try our best to avoid it being killed. This is just a coding style.

If you want to know other side-effects, I must say I have not researched about it. I just patched the code, packaged the extension, then loaded it in firefox. And the extension seems working.

Considered that this script is just exchanging data between dbus and firefox, and those functions are all still not changed, I think there may no other side-effect?

[kalvdans]

It is because Mozilla's documentation says firefox will send SIGTERM to its extensions' native app part when the extension is closing.

Thanks! Do you have a link to the documentation to add in the source code?

I agree. But we have a chance to quit program gracefully instead wait for being killed now, I think we'd better try our best to avoid it being killed. This is just a coding style.

I disagree, if we are not aware of any needed cleanup side-effects, adding extra code is unnecessary burden at this moment. I'm a fan of crash-only software :)

[arenekosreal]

Do you have a link to the documentation to add in the source code?

Not in source code, but in commit message.

I'm a fan of crash-only software

Wow, you are the first one I know that like this style. It seems that our conversation is just about different code style. Let maintainer decide if there is any change needed.