search

fleetdm / fleet

Open-source platform for IT, security, and infrastructure teams. (Linux, macOS, Chrome, Windows, cloud, data center)
https://fleetdm.com
Other
2.67k stars 379 forks source link

Make the mocking generated code thread-safe #4240

Open mna opened 2 years ago

mna commented 2 years ago

This is more a tech debt issue that was raised by this PR/comment: https://github.com/fleetdm/fleet/pull/4230#issue-1140068620 and discussed in the backend team meeting.

Currently, we use https://github.com/groob/mockimpl and it works well enough, but when used in a multi-threaded scenario, the code it generates is not thread-safe (it assigns a boolean flag indicating that the mocked method was called, without synchronization mechanism).

The fix is simple enough - each mocked method could have an unexported atomic integer associated with it, and the integer would be atomically incremented on each call. Instead of the ...Invoked field to check if a method has been called, there could be a ...Invoked() bool method that would simply return atomic.LoadUint64(theAtomicInteger) > 0, and there could be a ...Reset() method generated for each mocked method to safely reset it to not-called so users of the mock never have to remember to use atomic operations.

Whether we get that change upstream or we fork the repo is TBD, it would break the API so the upstream maintainer might not want the change, but we should reach out to them first. This would result in minimal changes to adjust our tests, and the PR #4230 could be undone (the change was specific for a single scenario/test).

This is not high priority - in fact, we shouldn't make any changes until we face the need to have a thread-safe mock elsewhere, as currently we only need it in a single place.

roperzh commented 1 year ago

adding another case where this bit us to the list: https://github.com/fleetdm/fleet/pull/7952