Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix race conditions and GH API endpoints #100

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

Conversation

theCalcaholic
Copy link

@theCalcaholic theCalcaholic commented Aug 23, 2023

I was investigating, why most of the stats in the overview image were 0 lately.

Turns out, there was a race condition in the properties awaiting get_stats().

All of these properties would check a field that is supposed to be filled by get_stats() and if it was None, they would await the result of get_stats(). However, since these fields were set to empty values (e.g. dict()) at the beginning of get_stats(), some of the async properties of Stats would return these empty values before get_stats() was actually completed, resulting in lots of zeros in the generated images :)

So what I changed is this:

  1. Set the property values (e.g. Stats._lines_changed, Stats._name ...) at the end of get_stats() rather than the beginning
  2. Make sure that all async properties will wait for get_stats() to be completed, by introducing an asyncio.Event and wrapping get_stats in a function that ensures it is only called once and otherwise awaits the event.

This is the relevant code:

async def get_stats(self) -> None:
    if self._get_stats_completed is not None:
        await self._get_stats_completed.wait()
        return

    self._get_stats_completed = asyncio.Event()
    await self._get_stats()
    self._get_stats_completed.set()

async def _get_stats(self) -> None:
    ....

So get_stats was renamed to _get_stats. The new function get_stats checks if the event Stats._get_stats_completed already exists. If not, it creates it and starts the wrapped function _get_stats, awaits it and then sets the event. If the event already exists, it just waits for it to be set.

@jstrieb
Copy link
Owner

jstrieb commented Sep 11, 2023

Whoa, this is an awesome find/fix. Thanks! I'd been getting some bug reports about the stats being 0, but had not been able to replicate it myself. As such, I'm really appreciative of this.

Haven't had a ton of time for this project, lately. But when I get a chance, I intend to give this a review and get it merged.

@Bigjango13
Copy link

Unfortunately, even with this change it's not working for me, it seems the actions are stuck as "Pending", it may be because of this: https://github.com/orgs/community/discussions/119603

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants