Potential race condition with the in memory store.

[elliotcourant]

I hope I am not creating clutter with this.

I think there is the potential for a race condition when a job is started at the same time that Shutdown(...) is called for the in memory store.

When shutdown is called it reads the context array without a mutex, which would be in a different go routine than the workers themselves that own the context. A context could be appended to this array while shutdown is being performed.

neoq/backends/memory/memory_backend.go

Lines 193 to 199 in a17ea73

	func (m *MemBackend) Shutdown(ctx context.Context) {
	for _, f := range m.cancelFuncs {
	f()
	}
	m.cancelFuncs = nil
	}

But a mutex is used when the contexts are added.

neoq/backends/memory/memory_backend.go

Lines 139 to 141 in a17ea73

	m.mu.Lock()
	m.cancelFuncs = append(m.cancelFuncs, cancel)
	m.mu.Unlock()

1. I think the mutex should be changed to a RWMutex with Shutdown acquiring a read lock on the array, and start acquiring a write lock on the array. This would prevent start from adding a new context and thus starting a new job during a shutdown. But does not prevent a job from starting immediately afterwards.
2. Do the contexts ever get cleaned up from this array? I don't see anything immediately that would remove items from the array after a job has completed. While I doubt these are of significant overhead would it be worth looking into a way to clean them up afterwards as well?

Happy to take on this work myself and contribute, really excited to start using this tool!

[elliotcourant]

I see that the context is only created per handler, not per job. So its very unlikely that a handler will "start" after the shutdown. And the likely-hood of a handler being added during a shutdown is also very low. Happy to tweak this in a PR or to just close it!

[acaloiaro]

I think you're right about this.

If it's not too tedious to trigger it with a test, I think that'd be a good start.

[elliotcourant]

Do you think if start was called on a handler after shutdown that some error should be raised or a panic?