westonpace commented on a change in pull request #9533:
URL: https://github.com/apache/arrow/pull/9533#discussion_r592556508
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -177,6 +179,94 @@ class TransformingGenerator {
std::shared_ptr<TransformingGeneratorState> state_;
};
+template <typename T>
+class SerialReadaheadGenerator {
+ public:
+ SerialReadaheadGenerator(AsyncGenerator<T> source_generator, int
max_readahead)
+ : state_(std::make_shared<State>(std::move(source_generator),
max_readahead)) {}
+
+ Future<T> operator()() {
+ if (state_->first) {
+ // Lazy generator, need to wait for the first ask to prime the pump
+ state_->first = false;
+ auto next = state_->source();
+ return next.Then(Callback{state_});
+ }
+
+ // This generator is not async-reentrant. We won't be called until the
last
+ // future finished so we know there is something in the queue
+ auto finished = state_->finished.load();
+ if (finished && state_->readahead_queue.IsEmpty()) {
+ return Future<T>::MakeFinished(IterationTraits<T>::End());
+ }
+
+ auto next_ptr = state_->readahead_queue.FrontPtr();
+ auto next = std::move(**next_ptr);
+ state_->readahead_queue.PopFront();
+
+ auto last_available = state_->spaces_available.fetch_add(1);
+ if (last_available == 0 && !finished) {
+ // Reader idled out, we need to restart it
+ ARROW_RETURN_NOT_OK(state_->Pump(state_));
+ }
+ return next;
+ }
+
+ private:
+ struct State {
+ State(AsyncGenerator<T> source_, int max_readahead)
+ : first(true),
+ source(std::move(source_)),
+ finished(false),
+ spaces_available(max_readahead),
+ readahead_queue(max_readahead) {}
+
+ Status Pump(const std::shared_ptr<State>& self) {
+ // Can't do readahead_queue.write(source().Then(Callback{self})) because
then the
+ // callback might run immediately and add itself to the queue before
this gets added
+ // to the queue messing up the order
+ auto next_slot = std::make_shared<Future<T>>();
+ auto written = readahead_queue.Write(next_slot);
+ if (!written) {
+ return Status::UnknownError("Could not write to readahead_queue");
+ }
+ *next_slot = source().Then(Callback{self});
+ return Status::OK();
+ }
+
+ // Only accessed by the consumer end
+ bool first;
+ // Accessed by both threads
+ AsyncGenerator<T> source;
+ std::atomic<bool> finished;
+ std::atomic<uint32_t> spaces_available;
Review comment:
It would be great if `Write` told you if the write you performed just
filled up the queue. Instead it tells you that your write was ignored. I'd
also need `PopFront` to tell you if the pop you performed just made space in an
otherwise full queue. Instead it returns void. I think one could write an
SPSC queue to work this way but the queue we have does not.
I need to be able to get at the information atomically because a callback
can be getting filled at the same time a reader is pulling from the queue and
there is room for race conditions there. Also, the docs for `SizeGuess`
mention that it only returns an estimate of the size and it may be off.
Example race condition.
PRECONDITIONS: A queue exists that has one spot left, reader and writer
approach at same time.
Write adds item to queue
Reader pulls item off queue
Reader grabs size, thinks it just pulled an item off a full queue (it did)
and pumps to restart reader
Writer grabs size, thinks it just wrote to a non-full queue and pumps to get
next item.
As soon as two pumps run at once the source is being pulled reentrantly
(possibly even sync-reentrant) and that is an error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
