Repository navigation
ARROW-16692: [C++] StackOverflow in merge generator causes segmentation fault in scan - #13691
Conversation
|
There is some high level description of the problem & the fix in https://issues.apache.org/jira/browse/ARROW-16692 |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
The longer term plan is to replace all this with the push-based plan, right? Or does this have to stay for dataset?
Yes, as part of ARROW-16072. |
|
Hmm, sadly https://github.com/apache/arrow/runs/7477126709?check_suite_focus=true seems relevant. Perhaps this is leading the merge generator to not properly discard its state. |
|
I investigated the failing ASAN run further today. I'm able to reproduce it on master so it is unrelated to this change. The root problem is the same one that motivates ARROW-16072. From the JIRA:
The end result of this is that some bits of the scanner keep running in the event of a failure or an abort. These pieces keep their state alive via shared_ptr so it doesn't actually cause any exceptions. However, it can lead to cases like this memory leak. The last test in ScannerTest will test a failed scan. This causes some bits to be left running. Then the test ends and the process shuts down and the thread pool shuts down immediately. This causes some cleanup thread tasks to fail to get scheduled or not run. As a result there is some leakage at shutdown. This is only a shutdown leak and so pretty minor. ARROW-16072 will tie cancellation into the scanner so that once a plan is aborted and the user waits for it to finish then nothing will be left running and there will be no possibility for this. Long story short, I think we can proceed with merging this PR. |
|
I've filed ARROW-17198 for the memory leak. |
|
Can we add an ASAN suppression temporarily before merging? |
|
(Also probably worth doing the same for Valgrind) |
5176792 to
bdf66fb
Compare
I added a |
|
Failure seems unrelated. I'm going to merge. |
|
Benchmark runs are scheduled for baseline = 4e37149 and contender = 898e12e. 898e12e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…on fault in scan (#13691) Fix for the merged generator to avoid a potential stack overflow Authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]>
Fix for the merged generator to avoid a potential stack overflow