ARROW-8732: [C++] Add basic cancellation API #9528

pitrou · 2021-02-18T19:20:06Z

In this model, a StopSource is instantiated by the consumer, which passes a corresponding StopToken to producer API(s).

github-actions · 2021-02-18T19:20:27Z

https://issues.apache.org/jira/browse/ARROW-8732

cpp/src/arrow/util/future.h

pitrou · 2021-03-03T19:27:06Z

@ursabot crossbow submit -g python

pitrou · 2021-03-03T19:27:57Z

@github-actions crossbow submit -g python

github-actions · 2021-03-03T20:03:22Z

Revision: 848e020cfaf234411d3a0167b58dc39c030823a4

Submitted crossbow builds: ursacomputing/crossbow @ actions-174

Task	Status
test-conda-python-3.6
test-conda-python-3.6-pandas-0.23
test-conda-python-3.7
test-conda-python-3.7-dask-latest
test-conda-python-3.7-hdfs-3.2
test-conda-python-3.7-kartothek-latest
test-conda-python-3.7-kartothek-master
test-conda-python-3.7-pandas-latest
test-conda-python-3.7-pandas-master
test-conda-python-3.7-spark-branch-3.0
test-conda-python-3.7-turbodbc-latest
test-conda-python-3.7-turbodbc-master
test-conda-python-3.8
test-conda-python-3.8-dask-master
test-conda-python-3.8-hypothesis
test-conda-python-3.8-jpype
test-conda-python-3.8-pandas-latest
test-conda-python-3.8-pandas-nightly
test-conda-python-3.8-spark-master
test-debian-10-python-3
test-fedora-33-python-3
test-ubuntu-18.04-python-3

pitrou · 2021-03-04T10:29:24Z

@westonpace @bkietz Feel free to review.

bkietz

This is looking great!

A few comments:

cpp/src/arrow/util/io_util.cc

cpp/src/arrow/util/io_util_test.cc

cpp/src/arrow/util/task_group.cc

cpp/src/arrow/util/task_group_test.cc

cpp/src/arrow/util/thread_pool_test.cc

bkietz · 2021-03-04T16:41:55Z

cpp/src/arrow/util/cancel.cc

Suggested change

}

} else {

DCHECK_EQ(impl_->requested_.load(), -1);

}

If you call Poll() twice, then the cancel_error corresponding to the signal will have been cached. So you can have both a cancel_error and a positive signal number.

cpp/src/arrow/util/cancel.cc

cpp/src/arrow/util/cancel_test.cc

bkietz · 2021-03-04T17:20:42Z

cpp/src/arrow/csv/reader.cc

Seems we might prefer to rewrite this constructor to take an IOContext

We must support the legacy TableReader::Make taking both a MemoryPool* and an IOContext.

pitrou · 2021-03-05T10:15:45Z

cpp/src/arrow/util/task_group.cc

Note to self: can use self_->stop_token_ instead.

pitrou · 2021-03-05T10:16:14Z

cpp/src/arrow/util/thread_pool.cc

Note to self: simply re-lock outside of scope? (to let task be destroyed naturally)

westonpace

I learned a lot about signals just reading this 😄

westonpace · 2021-03-05T18:10:58Z

cpp/src/arrow/csv/reader.cc

For an iterator, no. In the future, if this becomes a generator, possibly. The only way it could be useful is if the I/O could be cancelled somehow. Generally that's not possible. With some non-blocking I/O schemes you can at least give up on the user-land side of the I/O. For networked I/O it may be possible but I seem to recall you saying S3 had no such mechanism.

You could maybe make use of the stop token in the readahead to stop the readahead but that doesn't seem too urgent. It will fill up the readahead queue, and then, there will be no active references, and it will all be cleaned up. Although it may be nice to add a unit test for that scenario. I wonder if I could get a consumer side reference count of some kind and abort it as soon as all consumer references are lost 🤔 . I'll add a JIRA for it.

Indeed, the AWS SDK doesn't support cancellation.

Agreed about the readahead part. This can be done later. Thanks!

cpp/src/arrow/csv/reader.cc

westonpace · 2021-03-05T18:34:39Z

cpp/src/arrow/testing/gtest_util.cc

Nice. I was just thinking of writing something like this.

westonpace · 2021-03-05T19:07:58Z

cpp/src/arrow/util/cancel.cc

I think you can get rid of this mutex. If you changed this to something like...

if (!impl_->requested_.fetch_or(-1)) { impl_->signum = signum; }

signum would be a new (non-atomic) int member that replaces the mutex. Then RequestStop becomes...

if(!impl->requested_.fetch_or(-1)) { impl->cancel_error_ = std::move(st); }

The main advantage is there would be no way to lose your thread when calling Poll.

No, we need the mutex to protect cancel_error_ (which we cannot store atomically).

westonpace · 2021-03-05T19:25:20Z

cpp/src/arrow/util/cancel.cc

If someone calls ResetSignalStopSource while you are handling the signal wouldn't this become the sole reference and, on exit, attempt to destroy it (which would not be async-signal-safe)?

Admittedly, probably not a common occurrence.

Ouch, I think you're right. I'll try to find a way to do this differently...

cpp/src/arrow/util/cancel.h

cpp/src/arrow/util/cancel_test.cc

westonpace · 2021-03-05T20:02:28Z

python/pyarrow/_csv.pyx

Is there a JIRA to expose StopSource to python at some point? There are reasons other than signals someone may want to cancel an operation. For example, a GUI-based application may have a cancel button. A web server may want to cancel if the TCP connection for some analysis request is lost.

No JIRA yet. We can open one, or do it when needed.

In this model, a StopToken is instantiated by the consumer and passed to producer APIs.

pitrou · 2021-03-08T17:33:36Z

I pushed an improvement for the deallocation in signal handler problem, but it's still not 100% safe (though probably extremely unlikely). I think solving it entirely would need a lock-free doubly-linked list.

pitrou · 2021-03-08T19:06:23Z

@github-actions crossbow submit -g python

westonpace · 2021-03-08T20:46:22Z

Abseil (seems to be Google's equivalent of Folly) has an async-signal-safe spin lock. That might be easier than a doubly linked list. However, it doesn't seem to be easily vendored (just based on the fact that it has 10 abseil includes) :(

https://github.com/abseil/abseil-cpp/blob/master/absl/base/internal/spinlock.h

I'd agree that it seems too unlikely to worry about.

pitrou · 2021-03-08T20:50:14Z

How would a spinlock help the deallocation issue?

westonpace · 2021-03-08T21:00:04Z

The handler would obtain the lock before calling atomic_load on the pointer and reset the pointer before it releases the lock. Any call to ResetSignalStopSource would grab the lock before modifying the pointer. This should ensure that the pointer obtained by the load cannot go out of scope while the signal handler references it and thus would never have to free.

pitrou · 2021-03-08T21:04:29Z

Hmm, but resetting the pointer may still deallocate the underlying object... which is certainly not async-signal-safe.

westonpace · 2021-03-08T21:07:47Z

It could only deallocate it if the global reference was changed and all attempts to change the global reference should go through the same spinlock. The signal handler should always be given the "second reference" no matter what.

Or, since you're using spinlocks to guard all changes to the global reference, you could just change the global reference to a plain old pointer which might make it more obvious.

pitrou · 2021-03-08T21:10:53Z

Ah, I see: no need to take a strong reference inside the handler since the spinlock guarantees the global reference isn't mutated. That said, the signal-safe spinlock apparently requires one to block signals.

westonpace · 2021-03-08T21:18:52Z

Yep. I think what you have, with the comment, is fine.

github-actions bot added the Component: C++ label Feb 18, 2021

pitrou commented Feb 18, 2021

View reviewed changes

cpp/src/arrow/util/future.h Outdated Show resolved Hide resolved

pitrou marked this pull request as draft February 18, 2021 19:27

pitrou force-pushed the ARROW-8732-cancel-v2 branch 3 times, most recently from 27885ff to 5a05f3f Compare February 23, 2021 18:52

pitrou force-pushed the ARROW-8732-cancel-v2 branch 3 times, most recently from 7e9c191 to 5c81fd2 Compare March 1, 2021 19:23

github-actions bot added the Component: Python label Mar 1, 2021

pitrou force-pushed the ARROW-8732-cancel-v2 branch 8 times, most recently from a7d3d2f to e0d0092 Compare March 3, 2021 19:03

pitrou marked this pull request as ready for review March 3, 2021 19:05

pitrou mentioned this pull request Mar 3, 2021

ARROW-8732: [C++] Add basic cancellation API #7179

Closed

pitrou changed the title ~~ARROW-8732: [C++] Add basic cancellation API (v2)~~ ARROW-8732: [C++] Add basic cancellation API Mar 3, 2021

pitrou force-pushed the ARROW-8732-cancel-v2 branch from e0d0092 to 848e020 Compare March 3, 2021 19:39

bkietz requested changes Mar 4, 2021

View reviewed changes

pitrou force-pushed the ARROW-8732-cancel-v2 branch 2 times, most recently from a92611f to b82c13f Compare March 4, 2021 18:03

pitrou requested a review from bkietz March 4, 2021 18:48

pitrou commented Mar 5, 2021

View reviewed changes

cpp/src/arrow/util/task_group.cc Outdated

Copy link

Member Author

pitrou Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: can use self_->stop_token_ instead.

pitrou commented Mar 5, 2021

View reviewed changes

westonpace reviewed Mar 5, 2021

View reviewed changes

ARROW-8732: [C++] Add basic cancellation API

db498a8

In this model, a StopToken is instantiated by the consumer and passed to producer APIs.

pitrou force-pushed the ARROW-8732-cancel-v2 branch from b82c13f to db498a8 Compare March 8, 2021 17:22

pitrou closed this in 79ae4f6 Mar 9, 2021

pitrou deleted the ARROW-8732-cancel-v2 branch March 9, 2021 16:08

asfimport mentioned this pull request Mar 22, 2021

[C++] Let Futures support cancellation #17171

Closed

-  }
+  } else {
+    DCHECK_EQ(impl_->requested_.load(), -1);
+  }

ARROW-8732: [C++] Add basic cancellation API #9528

ARROW-8732: [C++] Add basic cancellation API #9528

Uh oh!

Conversation

pitrou commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 18, 2021

Uh oh!

Uh oh!

pitrou commented Mar 3, 2021

Uh oh!

pitrou commented Mar 3, 2021

Uh oh!

github-actions bot commented Mar 3, 2021

Uh oh!

pitrou commented Mar 4, 2021

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Mar 8, 2021

Uh oh!

westonpace commented Mar 8, 2021

Uh oh!

pitrou commented Mar 8, 2021

Uh oh!

pitrou commented Feb 18, 2021 •

edited

Loading

pitrou Mar 4, 2021 •

edited

Loading

pitrou Mar 5, 2021 •

edited

Loading

pitrou Mar 8, 2021 •

edited

Loading

pitrou commented Mar 8, 2021 •

edited

Loading