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

Potential inconsistencies updating settings #161

Closed
josecelano opened this issue May 18, 2023 · 1 comment
Closed

Potential inconsistencies updating settings #161

josecelano opened this issue May 18, 2023 · 1 comment

Comments

@josecelano
Copy link
Member

There is no mechanism to avoid race conditions while updating the settings.

Two admins can read the settings simultaneously, getting the settings at state S1.
Then they will POST the changes, and given we are using a RwLock to update the settings, one will be the first and the other the second. Changes from the first one will be lost. Before updating, we do not check that settings are still the same as the previous version, S1.

We can use optimistic or pessimistic locking.

Pessimistic locking

Before updating the settings, the admin should get the write lock. I suppose we have to add a "read for update" endpoint, so the frontend should make two requests:

  • Get settings for update (acquire the lock)
  • Update settings (release lock)

The lock could be the settings lock.

It would be a way to acquire the lock "remotely". It should have a timeout to release the lock.

Optimistic locking

We can add a "version" attribute to the settings. And we can increase the version number after updating the settings. Before updating, we can check that the version number is still the same.

Other considerations

Anyway, I would not update all the settings via an API endpoint. I would allow the admin to change only "runtime" configuration like the site name. And I would move those options to the database.

If we remove the enpoint, we no longer have these inconsistencies because you have to restart the service to change the settings. Settings would be read-only.

See: #144

cc @da2ce7 @WarmBeer

@josecelano
Copy link
Member Author

This does not make sense anymore because now we do not allow updating the settings.

You need to restart the application in order to update the settings.

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

No branches or pull requests

1 participant