flathub-infra / flatpak-external-data-checker

A tool for checking if the external data used in Flatpak manifests is still up to date
GNU General Public License v2.0
116 stars 34 forks source link

Hangs (sometimes) if a checker raises an assertion failure #420

Open wjt opened 2 months ago

wjt commented 2 months ago

In #418, @tobiasmicheler figured out that the checker was hanging while checking https://github.com/flathub/com.modrinth.ModrinthApp/blob/1d954cc7c91841d020fe4c860593789a16ca62c6/com.modrinth.ModrinthApp.yml#L108. This is because prior to 1a0e0c76449a20344584644cca69c6c61d494af2, gitchecker.GitChecker._check_has_new asserted that the (user-supplied) regular expression contained exactly one match group.

Stylistically I think it was incorrect to make an assertion about user-supplied data: assertions are to catch logic errors in the checker itself. But nonetheless it is the case that assertions are used liberally in the various checkers, so it's a problem that the asyncio mainloop would hang in this situation.

The symptom is that once the assertion is raised, the program hangs; if you interrupt it with Ctrl+C you get the following backtrace:

^CTraceback (most recent call last):
  File "/usr/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/sysroot/home/wjt/src/flathub-infra/flatpak-external-data-checker/src/main.py", line 465, in run_with_args
    await manifest_checker.check(args.filter_type)
  File "/sysroot/home/wjt/src/flathub-infra/flatpak-external-data-checker/src/manifest.py", line 408, in check
    ext_data_checked = await asyncio.gather(*check_tasks)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/sysroot/home/wjt/src/flathub-infra/flatpak-external-data-checker/src/manifest.py", line 343, in _check_data
    await checker.check(data)
  File "/sysroot/home/wjt/src/flathub-infra/flatpak-external-data-checker/src/checkers/gitchecker.py", line 92, in check
    return await self._check_has_new(external_data)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/sysroot/home/wjt/src/flathub-infra/flatpak-external-data-checker/src/checkers/gitchecker.py", line 101, in _check_has_new
    assert tag_re.groups == 1
           ^^^^^^^^^^^^^^^^^^
AssertionError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/sysroot/home/wjt/src/flathub-infra/flatpak-external-data-checker/./flatpak-external-data-checker", line 30, in <module>
    main()
  File "/sysroot/home/wjt/src/flathub-infra/flatpak-external-data-checker/src/main.py", line 504, in main
    outdated_num, errors_num, updated = asyncio.run(run_with_args(args))
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 189, in run
    with Runner(debug=debug) as runner:
  File "/usr/lib/python3.11/asyncio/runners.py", line 63, in __exit__
    self.close()
  File "/usr/lib/python3.11/asyncio/runners.py", line 71, in close
    _cancel_all_tasks(loop)
  File "/usr/lib/python3.11/asyncio/runners.py", line 201, in _cancel_all_tasks
    loop.run_until_complete(tasks.gather(*to_cancel, return_exceptions=True))
  File "/usr/lib/python3.11/asyncio/base_events.py", line 640, in run_until_complete
    self.run_forever()
  File "/usr/lib/python3.11/asyncio/base_events.py", line 607, in run_forever
    self._run_once()
  File "/usr/lib/python3.11/asyncio/base_events.py", line 1884, in _run_once
    event_list = self._selector.select(timeout)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/selectors.py", line 468, in select
    fd_event_list = self._selector.poll(timeout, max_ev)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyboardInterrupt

A bit of printf debugging shows that the gitchecker for another module is executing the async function git_ls_remote at the point of the hang. If I remove that module, the assertion bubbles up to the top level correctly.

gasinvein commented 2 months ago

Stylistically I think it was incorrect to make an assertion about user-supplied data: assertions are to catch logic errors in the checker itself. But nonetheless it is the case that assertions are used liberally in the various checkers,

Indeed, assertions in cases such as this one are used in place of proper input checks and should be replaced, probably with schema-based validation (I believe the number of capture groups in a regex can be checked with an extended python-jsonschema validator).

so it's a problem that the asyncio mainloop would hang in this situation.

And this is kinda weird. Are there some specific cases when AssertionError isn't propagated to the main loop? Or anywhere in a coroutine will cause it to hang?