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

caldav_alarm.c: switch from using mboxname to mboxid + inboxid #4842

Open
wants to merge 2 commits into
base: mboxlist_inboxid
Choose a base branch
from

Conversation

ksmurchison
Copy link
Contributor

Requires mboxlist_inboxid

@ksmurchison ksmurchison requested review from brong and rsto March 12, 2024 06:02
Copy link
Member

@brong brong left a comment

Choose a reason for hiding this comment

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

Looks like there's a memory leak...

imap/user.c Show resolved Hide resolved
Copy link
Member

@rsto rsto left a comment

Choose a reason for hiding this comment

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

Couldn't the upgrade code have race conditions with a concurrent user rename? Or maybe I just don't see how the code prevents that.

imap/caldav_alarm.c Show resolved Hide resolved
@ksmurchison
Copy link
Contributor Author

@brong Regarding @rsto's question about a race. The upgrade code runs the first time caldav_alarm_open() is called.
So, we could either have the upgrade code grab an exclusive lock on all of mailboxes.db, or we could enable wait daemon mode for calalarmd and only signal to master that its ready after the first call to caldav_alarm_process(), or we could tell admins to run calalarmd -U' prior to starting up Cyrus.
Or, any other ideas?

@ksmurchison ksmurchison requested review from brong and rsto March 13, 2024 02:53
@elliefm
Copy link
Contributor

elliefm commented Mar 14, 2024

So, we could either have the upgrade code grab an exclusive lock on all of mailboxes.db, or we could enable wait daemon mode for calalarmd and only signal to master that its ready after the first call to caldav_alarm_process()

We'd probably need to do both anyway, because wait daemon mode doesn't guarantee the daemon is only run during startup, only that when it is run during startup, it runs before services and other daemons start. If the daemon crashes or otherwise exits once services are already running, master will just restart it like a normal daemon. (Though presumably at that point the upgrade has already run, so there's no problem... as long as the upgrade isn't the cause of the crash/exit.)

We'd need to instruct people to add wait=y to their calalarmd entry, and some won't read or follow that instruction, and potentially trip over something later.

or we could tell admins to run calalarmd -U prior to starting up Cyrus.

Same potential issue wrt people upgrading without reading instructions.

Can we make ctl_cyrusdb -r take care of it, like we did with the uuid mailboxes upgrade? Everyone should already be running that from START, so it runs and finishes before even waitdaemons.

@ksmurchison
Copy link
Contributor Author

Can we make ctl_cyrusdb -r take care of it, like we did with the uuid mailboxes upgrade? Everyone should already be running that from START, so it runs and finishes before even waitdaemons.

Yes. We can probably just have it call caldav_alarm_upgrade().

@brong What do you think?

@rsto
Copy link
Member

rsto commented Aug 20, 2024

@ksmurchison I'm going to unassign my review status from this PR. Please add me for review again when this PR is ready for re-review.

@rsto rsto removed their request for review August 20, 2024 06:15
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.

None yet

4 participants