Allow storing arbitrary data in a job

[hanzei]

Summary

This PR allows plugins to store arbitrary data as part of their scheduled jobs. props is stored as part of the JobOnce struct in the KV store.

I tried implementing the changes using generics for type safety, but that didn't work out well with the current way the code is structured, as JobOnceScheduler is a global singleton.

The code should look like this:

type JobOnceScheduler[T any] struct {
[...]
}

var s *JobOnceScheduler[any]

// GetJobOnceScheduler returns a scheduler which is ready to have its callback set. Repeated
// calls will return the same scheduler.
func GetJobOnceScheduler[T any](pluginAPI JobPluginAPI) *JobOnceScheduler[T] {
	schedulerOnce.Do(func() {
		s = &JobOnceScheduler[T]{
			pluginAPI: pluginAPI,
			activeJobs: &syncedJobs{
				jobs: make(map[string]*JobOnce),
			},
			storedCallback: &syncedCallback{},
		}
	})
	return s
}

which doesn't work as JobOnceScheduler[T] can't be assigned to a JobOnceScheduler[any].

If this PR needs type safety, which would be nice, we would need to get rid of the singleton. That could be done by prefixing the key of each scheduler, a pattern that is used in other packages of pluginapi.

Ticket Link

Needed for mattermost/mattermost-plugin-apps#375

[levb]

LGTM other than 1 nit

[hanzei]

@levb Heads up that I added some thoughts about using generics.

[agarciamontoro]

Looks good! Just one nit below. I'd also want @cpoile to take a look, since he worked on this initially and he'll have the best context to review the PR.

[cpoile]

This is interesting! I think we might need to think a second though before merging, just in case --

For a bit of context -- the reason we didn't include a props in the first version of this was that the key the job returns can be the key in the kvstore -- so if you want to store a "props" you would store them in the kvstore with that key and pull it out once the jobonce fires. Which means less data trapped in goroutines...

This PR might let people think they can put anything into Props, which could cause some problems if they do put anything in there...

Also, I haven't looked at Go generics yet, but do we have a way (e.g., type bounds?) to ensure that Props is json.Marshal and json.Unmarshal-able?

[hanzei]

This PR might let people think they can put anything into Props, which could cause some problems if they do put anything in there...

It's worth noting that the KV store methods like Set (

mattermost-plugin-api/kv.go

Line 58 in 24f6408

func (k *KVService) Set(key string, value interface{}, options ...KVSetOption) (bool, error) {

) also don't do a good job in documenting what types can be passed as values.

Given that encode/json.Marshal is used in the KV store, the documentation does also apply here. Marshal returns a UnsupportedTypeError if a value type is unsupported. The code propagates that error all the way to the top. Hence, that's an error that should be spotted during development.

Also, I haven't looked at Go generics yet, but do we have a way (e.g., type bounds?) to ensure that Props is json.Marshal and json.Unmarshal-able?

I can't think of a way. Generics in go don't allow recursive type parameters. Hence, struct as a type argument has to contain a finite list of fields.

[cpoile]

It's worth noting that the KV store methods like Set (...) also don't do a good job in documenting what types can be passed as values.

True, but I think the real issue is whether we want to put big chunks of data in the job once goroutines. This was meant to be lightweight. The way it's designed now, there can be an unlimited number of long running goroutines parked waiting for their timer to expire.

If we really want to attach data to the job once items, I think we need to redesign how the system schedules and parks its future events. I think @lieut-data had a good idea: we could use a priority queue and periodically poll the db to add soon-to-fire items to that queue. That way we would have only the next x minutes of items parked at any one time.

If we moved to something like that, I think there might be less chance for memory issues when users store arbitrary amounts of data.

Or, alternatively, set a max number of bytes and enforce that at runtime?

[hanzei]

/update-branch

[codecov]

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.30 🎉

Comparison is base (2b94f6c) 72.82% compared to head (7c180dc) 73.13%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   72.82%   73.13%   +0.30%     
==========================================
  Files          33       33              
  Lines        1774     1783       +9     
==========================================
+ Hits         1292     1304      +12     
+ Misses        418      416       -2     
+ Partials       64       63       -1     

Impacted Files	Coverage Δ
cluster/job_once.go	85.60% <72.72%> (+1.11%)	⬆️
cluster/job_once_scheduler.go	80.30% <100.00%> (+2.27%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[hanzei]

@cpoile A priority queue is an awesome idea to reduce the memory consumption of the job scheduler. But it's a bit out of scope of this PR. I think it would make for a great HW ticket!

As part of this PR I've added a limit of 10k bytes for props in e50c0fb. That should prevent enormous, long-lasting go routines.

[cpoile]

Cool, I think that's a reasonable compromise since parked routines are about 4k.

I really hope people don't fill these props up though, that would have unexpected results if they don't understand what's really happening (every one is parked until it runs).