codex-storage / questionable

Elegant optional types for Nim
Other
115 stars 5 forks source link

`errorVariable` and thread safety #45

Closed elcritch closed 1 year ago

elcritch commented 1 year ago

While digging into https://github.com/codex-storage/questionable/issues/41 I noticed that binderror uses a global to store the last error state:

https://github.com/codex-storage/questionable/blob/e56cf86c4a089c78a1b7c3005f13343bfbbe3b48/questionable/private/binderror.nim#L4C10-L4C10

It should probably be made into var errorVariable: ptr ref CatchableError.

If it's not a threadvar issues might arise when used with multiple threads. I haven't worked through the precise logic, but some sequence similar to:

Probably not all too likely and shouldn't cause a false error case, just report the wrong error. That'd still be annoying.

markspanbroek commented 1 year ago

Good find, thanks! I was able to reproduce this in a test and adding {.threadvar.} indeed fixes the problem: #46