status-im / nim-chronos

Chronos - An efficient library for asynchronous programming
https://status-im.github.io/nim-chronos/docs/chronos
Apache License 2.0
352 stars 51 forks source link

Completing `Futures` in refc may have unexpected behavior #511

Open gmega opened 4 months ago

gmega commented 4 months ago

I've recently bumped into an issue that affects how sink parameters behave in refc and which can have adverse effects on Chronos' (V4) futures: sink parameters can end up being passed by reference even when they should have been copied; i.e., even when there are later accesses to the same location at the caller.

This can in turn cause surprising behavior as the move in chronosMoveSink resets the location of the parameter, causing unexpected results:

import chronos

type 
  AnObject = object of RootObj
    inner: int

let fut = newFuture[AnObject]("myFuture")
var anObject = AnObject(inner: 42)

fut.complete(anObject)

echo fut.value.inner, ", ", anObject.inner   

this prints "42, 0" when it should print "42, 42". If AnObject held a ref object in inner, you'd crash with a SIGSEV.

For async procs this seems to be less of an issue as capturing the parameter in a closure seems to get it copied somehow (I still don't get the whole mechanics), plus complete would typically only get called after user code is done so even accidental mutation is not the end of the world. But for code using "naked" futures like that one it's definitely an issue.

Assuming we're not abusing the API by using futures this way, one way around it would be disabling sinks for refc until this gets fixed.

arnetheduck commented 3 months ago

https://github.com/status-im/nim-chronos/pull/524 removes sink while this is investigated upstream