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

Events: Periodically switch out of the event parsing loop #686

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

squeaky-pl
Copy link
Contributor

@squeaky-pl squeaky-pl commented Feb 27, 2024

Normally these loops parse a very limited number of events, but at the initial sync the implementation will fetch all the events into a list and then start parsing them all. This might block the loop for a little too long and result in the calendar syncing greenlet being interrupted and killed.

@squeaky-pl squeaky-pl changed the title Events: Periodically witch out of the event parsing loop Events: Periodically switch out of the event parsing loop Feb 27, 2024
@@ -92,7 +93,7 @@ def sync_events(
updates = []
items = self._get_raw_events(calendar_uid, sync_from_time)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if sync_from_time is way back in the past, then _get_raw_events may return millions of events here? This isn't just a yield-to-the-loop-every-now-and-then kind of a problem – it's also a memory problem, right?

Also, nit: items is too vague a name for something that we have a clear, precise name for. Maybe rename to raw_events?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is true. The original Google implementaiton "hoards" the events into a single list, while it could actually perfectly yield them one by one. Microsoft one does not have this problem as it uses generators all the way down the stack. I will port Google implementation to generators as well on a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change the name as well here.

@squeaky-pl squeaky-pl merged commit e84a084 into master Feb 27, 2024
5 checks passed
@squeaky-pl squeaky-pl deleted the events-periodically-switch branch February 27, 2024 11:34
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.

2 participants