alphagov/router

Use sync/atomic for atomicity

Closed this issue · 2 comments

Hi!

I believe you need to use sync/atomic to implement some of the operations here. Since you are reading/writing this variable concurrently, you should be using sync.{Load,Store}Pointer to ensure atomicity, this will avoid torn reads.

Unfortunately, this is somewhat ugly; you have to use unsafe.Pointer to use the sync/atomic API. I recommend writing an atomicSetMap() and atomicGetMap() functions to contain the ugliness.

@dvyukov can probably tell me if I'm being too paranoid.

Hi,

thanks for the feedback on this. This has cause me to do quite a lot more reading around this area, and I think the reason this hasn't been a problem for us is because we're running exclusively on x86/amd64 platforms.

Nevertheless, as this code is not guaranteed to be correct under the Go memory model, I've added a mutex to protect access to this pointer in #67. This seemed to be cleaner than going down the unsafe.Pointer route.

Since the potential race condition mentioned is now prevented by #67 and there has been no further feedback, I'm going to close this issue.

Thanks for raising this @balasanjay! 👍