DavisVaughan
commented
1 year ago My guess as to what happens is:
- The
inputwas being copied as it was sent to the thread, and stored in the thread "shared state" - The copy is destroyed at a later time, likely at the
get()time, due to being part of the thread "shared state"
Now, these get() calls end up happening in another async function
https://github.com/tidyverse/vroom/blob/3691c6833006d319b2edca378258333fb9161135/src/vroom_write.cc#L406
Which means that, at least in theory, I think future[0] could be calling get() and running a cpp11 destructor (doing a cpp11 release()) at the same time future[1] is copying input (doing a cpp11 insert()). Because the insert/delete behavior share the same underlying SEXP preserve list, I think we are occasionally accessing it at the exact same time across threads, which isn't safe.
Very good explanation of async argument destruction time: https://stackoverflow.com/questions/47436919/why-is-passing-by-const-ref-slower-when-using-stdasync
I am still not 100% sure why fiddling with the preserve list on the cpp11 side (that commit where stuff started breaking) triggered this to show up, but I am now fairly confident that vroom is where the fix should come from.
jennybc
Fixes #510
I don't think we should merge this yet, but I do think it should fix the issue.
Has something to do with the fact that
std::asyncwill force a copy of the arguments you pass it, even if they ultimately get called infasconst reference, becausestd::asynchas no way of knowing that.So to perfect forward a const reference, the right way to do it is to wrap the thing you want a reference to in
std::ref()and pass that along instead.I am not entirely sure why this broke with new cpp11. Probably some cpp11 destructor is being run (on the thread?) that unprotects the underlying SEXP too early.
https://stackoverflow.com/questions/18359864/passing-arguments-to-stdasync-by-reference-fails https://www.linkedin.com/pulse/careful-when-using-const-reference-stdthread-stdasync-bhavith-c-achar