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

Add error handling to flatpak_system_updates getter #44

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Tropingenie
Copy link

Fixes #43.

The only "new" code is the block:

        error = True # No do-while in Python so init to true to run loop once
        while not error:
            try:
                flatpak_system_updates = self.system_installation.list_installed_refs_for_update(None)
            except gi.repository.GLib.GError as e:
                logger.error(e)
            else:
                error = False

I ended up using the with statement to reduce repeated code. However, you may find it simpler to either:

  • Copy paste the while loop in the two places flatpak_system_updates is used
  • Use the @contextmanager decorator from contextlib to use a "function" instead of a "class"

I also ended up making the naming more consistent between the two functions that used the flatpak_system_updates variable.

You may also want to add some sort of timeout to the loop to prevent infinite blocking (e.g. if the user runs nobara-sync with no internet I feel like the error would not go away)

Need to verify that del self.system_installation actually works and doesn't introduce a memory leak.

No code quality guideline was provided so I used my rusty PEP-8 muscle memory. Seems to all look close enough to the existing code.

Also, complete disclaimer, this code is untested as I do not have a build environment nor can I reliably reproduce the error. Reading through, unless stated above I don't see any issues but the lack of testing is still a concern.

@GloriousEggroll
Copy link
Contributor

GloriousEggroll commented Feb 17, 2025

gave this a quick test, it crashes after checking repositories:

$ nobara-sync
2025-02-17 15:37:05 - INFO - Running GUI mode...
2025-02-17 15:37:05 - INFO - Checking repositories...

2025-02-17 15:37:06 - INFO - Last metadata expiration check: 0:01:26 ago on Mon 17 Feb 2025 03:35:40 PM MST.
2025-02-17 15:37:12 - INFO - ✔ fedora-cisco-openh264: metalink: https://mirrors.fedoraproject.org/metalink?repo=fedora-cisco-openh264-41&arch=x86_64

2025-02-17 15:37:12 - INFO - ✔ fedora-cisco-openh264-multilib: metalink: https://mirrors.fedoraproject.org/metalink?repo=fedora-cisco-openh264-41&arch=i386

2025-02-17 15:37:12 - INFO - ✔ rpmfusion-free: metalink: https://mirrors.rpmfusion.org/metalink?repo=free-fedora-41&arch=x86_64

2025-02-17 15:37:12 - INFO - ✔ rpmfusion-free-updates: metalink: https://mirrors.rpmfusion.org/metalink?repo=free-fedora-updates-released-41&arch=x86_64

2025-02-17 15:37:12 - INFO - ✔ rpmfusion-nonfree: metalink: https://mirrors.rpmfusion.org/metalink?repo=nonfree-fedora-41&arch=x86_64

2025-02-17 15:37:12 - INFO - ✔ rpmfusion-nonfree-updates: metalink: https://mirrors.rpmfusion.org/metalink?repo=nonfree-fedora-updates-released-41&arch=x86_64

2025-02-17 15:37:12 - INFO - ✔ nobara-appstream: mirrorlist: https://mirrors.nobaraproject.org/rolling/appstream

2025-02-17 15:37:12 - INFO - ✔ nobara-updates: mirrorlist: https://mirrors.nobaraproject.org/rolling/baseos

2025-02-17 15:37:12 - INFO - ✔ nobara: mirrorlist: https://mirrors.nobaraproject.org/rolling/nobara

2025-02-17 15:37:12 - INFO - ✔ copr:copr.fedorainfracloud.org:patrickl:yabridge: baseurl: https://download.copr.fedorainfracloud.org/results/patrickl/yabridge/fedora-41-x86_64/repodata/repomd.xml

2025-02-17 15:37:12 - INFO - ✔ copr:copr.fedorainfracloud.org:phracek:PyCharm: baseurl: https://download.copr.fedorainfracloud.org/results/phracek/PyCharm/fedora-41-x86_64/repodata/repomd.xml

2025-02-17 15:37:12 - INFO - ✔ hardware_razer: baseurl: https://download.opensuse.org/repositories/hardware:/razer/Fedora_41/repodata/repomd.xml

2025-02-17 15:37:12 - INFO - ✔ repo.nordvpn.com_yum_nordvpn_centos_x86_64: baseurl: https://repo.nordvpn.com/yum/nordvpn/centos/x86_64/repodata/repomd.xml

2025-02-17 15:37:12 - INFO - Last metadata expiration check: 0:00:03 ago on Mon 17 Feb 2025 03:37:09 PM MST.
Exception in thread Thread-1 (run_updater):
Traceback (most recent call last):
  File "/usr/lib64/python3.13/threading.py", line 1041, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/usr/lib64/python3.13/threading.py", line 992, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/bin/nobara-updater", line 1396, in run_updater
    self.textview_updates()
    ~~~~~~~~~~~~~~~~~~~~~^^
  File "/usr/bin/nobara-updater", line 1263, in textview_updates
    result = check_updates(return_texts=True)
  File "/usr/bin/nobara-updater", line 497, in check_updates
    fp_system_updates = fp_get_system_updates()
  File "/usr/bin/nobara-updater", line 529, in fp_get_system_updates
    with fp_system_installation_list(system_installation) as flatpak_sys_updates:
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/usr/bin/nobara-updater", line 568, in __enter__
    return flatpak_system_updates
           ^^^^^^^^^^^^^^^^^^^^^^
UnboundLocalError: cannot access local variable 'flatpak_system_updates' where it is not associated with a value

note: if you want to test it on your own system first make sure nobara-updater is updated otherwise it's self-update mechanism may overwrite your changes, after that copy nobara_sync.py to/as /usr/bin/nobara-sync

@Tropingenie
Copy link
Author

note: if you want to test it on your own system first make sure nobara-updater is updated otherwise it's self-update mechanism may overwrite your changes, after that copy nobara_sync.py to/as /usr/bin/nobara-sync

Cheers, will actually test my code before updating this PR 😆

@Tropingenie
Copy link
Author

Alright, pushed changes that allow the updater to successfully run. This should test most of the code path of my changes, but I will continue to monitor until a Flatpak update comes up.

I also tested disconnecting from the internet and running the updater, and as expected it deadlocks. Will need to add a "timeout" to stop the loop after a few attempts.

system_installation needs to be deleted manually using del for
cleanup to occur. Nobara-Project#44 added a context manager in 9709fbc
however did not properly clean up refcount. This change removes
spurious references, and reintroduces `del` where required.
@Tropingenie
Copy link
Author

Tropingenie commented Feb 21, 2025

Have resolved everything I set out to do, ready for code review.

I fixed the deadlock by simply re-raising any errors. This restores the same behaviour as the latest release of nobara_sync as masking the error is out of scope (and was done so unintentionally). A timeout loop is not required, as a different error is raised when no network is available.

All references to system_installation should be properly removed now, as anywhere a reference is declared I have ensured a del follows. All other references are as a result of function calls passing a reference and should be automagically decremented.

Code has been tested in the non-updating path as of 977a17d, and the updating path as of 6ac0ff4, and found working on my machine. The changes between those commits are so small it is unlikely any error was introduced, and the context manager is run on both code paths regardless.

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