[C++][R] Async streaming CSV reader deadlocking when being run synchronously from datasets

[asfimport]

ARROW-11887 added async to the streaming CSV reader.  In order to keep backwards compatibility the old sync API simply calls the async API and waits for it to finish.  However, that wait cannot happen safely in a "nested" context (e.g. dataset reading).

For example, imagine two cores.  The dataset read launches two CSV scans.  Each scan occupies a core waiting for a future.  Those futures are being filled by I/O threads.  The I/O threads finish and go to transfer.  The transfer cannot happen because the CPU executor is filled.

This will be fixed as part of ARROW-7001 but that still some ways away.  An easier change might be to take some of the 7001 changes and include them as part of the 11887 feature.

Reporter: Weston Pace / @westonpace
Assignee: Weston Pace / @westonpace

PRs and other links:

• GitHub Pull Request #10018
• GitHub Pull Request #10019
• GitHub Pull Request #9868

_{Note: This issue was originally created as ARROW-12161. Please see the migration documentation for further details.}

[asfimport]

Weston Pace / @westonpace:
So there are a few choices here.  In the meantime, if it is going to take some time to fix, I'd recommend reverting ARROW-11887 while the fix is worked through.  I've created #9859 as a convenience.

Option #1: Leave 11887 out, include it as part of ARROW-7001

-Pros: No wasted work

-Cons: Makes ARROW-7001 an even larger change.

 

Option #2: Bring part of ARROW-7001 in and patch it in.  Basically add supports_async() and ExecuteAsync() to ScanTask and then modify Scanner::ToTable so that it will create a task group (for synchronous scan tasks) AND collect a set of futures (for async scan tasks).  It will then await both of those one after the other.  This should avoid the nested dataset issue.  I've prototyped this today and it should work but it'll take me a little bit of work to polish it which I could do tomorrow.

-Pros: Makes ARROW-7001 a smaller change

-Cons: Potentially delays ARROW-7001 review while this is worked through / Some wasted work.

[asfimport]

Antoine Pitrou / @pitrou:
Can you explain what work would be wasted in option 2?

[asfimport]

Weston Pace / @westonpace:
Minor things.  The concept (in Scanner::ToTable) that some tasks would be async while others are sync (in 7001 they are all async but I can't make that change without pulling a lot more of 7001 in).  A lot of the logic in scanner_internal I can't easily pull over from 7001 because I reworked what was in there pretty heavily but I will need it.  So this will mean kind of cherry picking bits and pieces from 7001 to put into scanner_internal.

[asfimport]

Antoine Pitrou / @pitrou:
Well, as you prefer, then. But avoiding making ARROW-7001 too large is a worthwhile goal IMHO.

[asfimport]

Weston Pace / @westonpace:
Ok, I will take another stab at it after I get the tests for 7001 back into a stable state.  But that may not be until tomorrow.  I'm not sure if you want to revert 11887 in the meantime or not.  The R builds will hang 100% of the time until this is addressed.

[asfimport]

David Li / @lidavidm:
Issue resolved by pull request 9868
#9868

[asfimport]

Weston Pace / @westonpace:
Tagging as a blocker because RTools 3.5 CI builds are failing due to this.  Can be addressed by ARROW-12208 or by disabling dataset scanning.

[asfimport]

David Li / @lidavidm:
The fix here was reverted in #10019 but the original issue is no longer present due to other fixes.