Fixes for typedthreads data race issues #24612
Conversation
Modifications have been tested with examples in the open issue. All seem to work except one with Thread[Socket] because of ref SocketImpl. Needs testing in Linux and Windows, probably better with some existing software that uses typedthreads to make sure I have not broken it.
|
fixes #24591 |
|
cpp targets fail to compile, probably needs this: https://en.cppreference.com/w/cpp/utility/functional/reference_wrapper |
lib/std/typedthreads.nim
Outdated
| when TArg isnot void: t.data = param | ||
| t.dataFn = tp | ||
| when TArg isnot void: t.data.store param | ||
| t.dataFn.store tp |
There was a problem hiding this comment.
these loads/stores can use relaxed memory ordering
There was a problem hiding this comment.
I set moAcquireRelease, and if I'm right, this will work just like withLock
| @@ -149,9 +149,9 @@ else: | |||
| nimThreadProcWrapperBody(closure) | |||
| {.pop.} | |||
|
|
|||
| proc running*[TArg](t: Thread[TArg]): bool {.inline.} = | |||
| proc running*[TArg](t: var Thread[TArg]): bool {.inline.} = | |||
There was a problem hiding this comment.
the var here is a breaking change, why is it needed?
There was a problem hiding this comment.
please ignore. If I manage to fix other issues that arise, this change will be removed with the next commit.
There was a problem hiding this comment.
the
varhere is a breaking change, why is it needed?
nim c --styleCheck:usages --styleCheck:error --verbosity:0 --hints:off --skipParentCfg --skipUserCfg --outdir:build '--nimcache:build/nimcache/$projectName' -d:metricsTest -d:metrics --threads:on -d:nimTypeNames --mm:refc -r tests/chronos_server_tests
/Users/runner/work/Nim/Nim/pkgstemp/metrics/metrics/chronos_httpserver.nim(400, 6) template/generic instantiation of async from here
/Users/runner/work/Nim/Nim/pkgstemp/metrics/metrics/chronos_httpserver.nim(403, 26) template/generic instantiation of running from here
/Users/runner/work/Nim/Nim/lib/std/typedthreads.nim(155, 22) Error: type mismatch
Expression: load(t.dataFn, moAcquireRelease)
[1] t.dataFn: Atomic[proc (m: MetricsServerData){.nimcall, gcsafe.}]
[2] moAcquireRelease: MemoryOrder
Expected one of (first mismatch at [position]):
[1] proc load[T: Trivial](location: var Atomic[T];
order: MemoryOrder = moSequentiallyConsistent): T
[1] proc load[T: not Trivial](location: var Atomic[T];
order: MemoryOrder = moSequentiallyConsistent): T
expression 't.dataFn' is immutable, not 'var'
|
For the cpp target, the old flow without atomics is retained. |
fixes #24591
Modifications have been tested with examples in the open issue 24591. All seem to work except one with Thread[Socket] because of ref SocketImpl. Needs testing in Linux and Windows, probably better with some existing software that uses typedthreads to make sure I have not broken it.