Make access to KV store atomically safe when saving/removing subscriptions

[mickmister]

Summary

For subscriptions of GitHub projects to MM channels, all of the subscription configuration is stored in the subscriptions portion of the KV store slice that is used by the Github plugin. In order to avoid race conditions and conflicts when two users are concurrently editing subscriptions, we should wrap the operations in a function like atomicModify used in the Jira plugin.

Ticket Link

https://mattermost.atlassian.net/browse/MM-18722

---

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Plugin: GitHub" community channel.

New contributors please see our Developer's Guide and our Plugins Guide.

[gsagula]

I'll take this one.

[gsagula]

@mickmister @hanzei Quick update: This issue is part of my tryout but I won't be able to start working on it until Oct 28.

cc: @jwilander

[hanzei]

Thanks for the heads up @gsagula. Looking forward to you working on this :+1:

[gsagula]

@mickmister @hanzei Quick question before I start working on this issue. If I understand it correctly, atomicModify function should only attempt to modify the subscriptions when the key exists in the KV storage and the initial value is equal to the current value. I'm not sure why true is being passed to success here:

success := false
for !success {
	initialBytes, newValue, err := readModify()
	if err != nil {
		return err
	}

	var setError *model.AppError
	success, setError = p.API.KVCompareAndSet(key, initialBytes, newValue)
	success = true
	if setError != nil {
		return errors.Wrap(setError, "problem writing value")
	}
}

Should this loop continue when p.API.KVCompareAndSet(key, initialBytes, newValue) returns false, nil, so that readModify can fetch new initial bytes and KVCompareAndSet can attempt to set the modified bytes again?

[mickmister]

Hi @gsagula,

It seems that the success = true line may be a bug. This line was introduced before the switch to using api.KVCompareAndSet, which gives us a success return value, but the success = true line was kept as well, (before and after). It makes sense to me to use the success value returned from KVCompareAndSet instead of the hardcode below the call. @crspeller do you know if leaving the success = true line was intentional after the switch?

Also, there was discussion about making the function into a helper (including a retry limit), so multiple plugins can reuse the same code: https://github.com/mattermost/mattermost-plugin-jira/pull/61/files#r286163455 . This is probably a good route to take for this ticket. What are your thoughts @gsagula?

[gsagula]

Thanks @mickmister,

I agree with limiting retries. Anything that allows controlling bursts so we can minimize chances of resource starvation. Something like a token bucket should probably be enough. WDYT?

I'm still getting familiar with the design. Is helpers_kv.go a good place to implement the helper?

[mickmister]

Something like a token bucket should probably be enough. WDYT?

Looks like a great solution, looking forward to the implementation!

I'm still getting familiar with the design. Is helpers_kv.go a good place to implement the helper?

Yep :+1:

[crspeller]

@mickmister Ya that looks like a bug.

[levb]

@gsagula do we still need this ticket, or has it been (indirectly?) resolved?

[gsagula]

I need to finish https://github.com/mattermost/mattermost-server/pull/13002 before getting back to this one.

[hanzei]

https://github.com/mattermost/mattermost-server/pull/13805 has to be done first