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

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

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.

3 participants