Speculative fix for vroom issue

[DavisVaughan]

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::async will force a copy of the arguments you pass it, even if they ultimately get called in f as const reference, because std::async has 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

[jennybc]

Thanks, Davis! This has a great ratio of "size of fix" to "size of headache / anxiety about problem" 😅

[DavisVaughan]

My guess as to what happens is:

• The input was 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

vroom/src/vroom_write.cc

Line 406 in 3691c68

auto buf = futures[idx][i].get();

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]

It feels like I should merge this @DavisVaughan and move towards release. How do you feel about that?

[DavisVaughan]

Yes I agree