[PATCH seastar] input_stream: Fix possible infinite recursion in consume()

[Tomasz Grabiec]

It may happen if the consumer is slower than the underlying data
source.

Fixes #195.
---
core/iostream-impl.hh | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/core/iostream-impl.hh b/core/iostream-impl.hh
index b229f7a..48a1d67 100644
--- a/core/iostream-impl.hh
+++ b/core/iostream-impl.hh
@@ -146,12 +146,12 @@ template <typename CharType>
template <typename Consumer>
future<>
input_stream<CharType>::consume(Consumer& consumer) {
- for (;;) {
+ return repeat([&consumer, this] {
if (_buf.empty() && !_eof) {
return _fd.get().then([this, &consumer] (tmp_buf buf) {
_buf = std::move(buf);
_eof = _buf.empty();
- return consume(consumer);
+ return make_ready_future<stop_iteration>(stop_iteration::no);
});
}
future<unconsumed_remainder> unconsumed = consumer(std::move(_buf));
@@ -160,15 +160,16 @@ input_stream<CharType>::consume(Consumer& consumer) {
if (u) {
// consumer is done
_buf = std::move(u.value());
- return make_ready_future<>();
+ return make_ready_future<stop_iteration>(stop_iteration::yes);
}
if (_eof) {
- return make_ready_future<>();
+ return make_ready_future<stop_iteration>(stop_iteration::yes);
}
// If we're here, consumer consumed entire buffer and is ready for
// more now. So we do not return, and rather continue the loop.
// TODO: if we did too many iterations, schedule a call to
// consume() instead of continuing the loop.
+ return make_ready_future<stop_iteration>(stop_iteration::no);
} else {
// TODO: here we wait for the consumer to finish the previous
// buffer (fulfilling "unconsumed") before starting to read the
@@ -177,14 +178,14 @@ input_stream<CharType>::consume(Consumer& consumer) {
if (u) {
// consumer is done
_buf = std::move(u.value());
- return make_ready_future<>();
+ return make_ready_future<stop_iteration>(stop_iteration::yes);
} else {
// consumer consumed entire buffer, and is ready for more
- return consume(consumer);
+ return make_ready_future<stop_iteration>(stop_iteration::no);
}
});
}
- }
+ });
}

template <typename CharType>
--
2.5.5

[Commit Bot]

From: Tomasz Grabiec <tgra...@scylladb.com>
Committer: Avi Kivity <a...@scylladb.com>
Branch: master

input_stream: Fix possible infinite recursion in consume()

It may happen if the consumer is slower than the underlying data
source.

Fixes #195.

Message-Id: <1474545575-26837-1-gi...@scylladb.com>

---
diff --git a/core/iostream-impl.hh b/core/iostream-impl.hh

[Shlomi Livne]

backport to 1.3 - no ?

On Thu, Sep 22, 2016 at 3:07 PM, Commit Bot <b...@cloudius-systems.com> wrote:

From: Tomasz Grabiec <tgra...@scylladb.com>
Committer: Avi Kivity <a...@scylladb.com>
Branch: master

input_stream: Fix possible infinite recursion in consume()

It may happen if the consumer is slower than the underlying data
source.

Fixes #195.

Message-Id: <1474545575-26837-1-git-send-email-t...@scylladb.com>

--

You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev+unsubscribe@googlegroups.com.
To post to this group, send email to seast...@googlegroups.com.
Visit this group at https://groups.google.com/group/seastar-dev.
To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/94eb2c0760f610cf88053d17824f%40google.com.

For more options, visit https://groups.google.com/d/optout.

[Tomasz Grabiec]

On Thu, Sep 22, 2016 at 2:28 PM, Shlomi Livne <shl...@scylladb.com> wrote:
> backport to 1.3 - no ?

I'm on it.

[Nadav Har'El]

Sorry about that, guys (Avi actually wrote this code initially, but I overhauled it and should have caught this).

I made a note to myself to explain in the Seastar tutorial why recursion is *not* a good replacement for Seastar's iteration primitives (do_until, repeat, etc.).

That being said, there is something I didn't fully understood here:
The code does something like this:

future<> consume() {

           return fd.get().then([] { return consume(); });
}

So if fd.get() is not ready, we don't have recursion here at all - consume() returns a future immediately, and sometime later will be called again (no recursion).

So recursion only happens if fd.get() is ready immediately.

But when this code is run immediately, then because of the "return" and the compiler could figure out that the code doesn't need any of the on-stack variables, the compiler could do "tail call optimization" to *replace* the last frame, instead of add to it, and avoid recursion. Why doesn't the compiler do it? Is it because C++ destruction order reasons (e.g., C++ thinks it needs to guarantee destruction of objects after then() returns, not before? Or just optimizer limitations?

Anyway, sure it's better to avoid relying on the tail call optimization...

--
Nadav Har'El

n...@scylladb.com

--
2.5.5

--
You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev+unsubscribe@googlegroups.com.
To post to this group, send email to seast...@googlegroups.com.
Visit this group at https://groups.google.com/group/seastar-dev.

To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/1474545575-26837-1-git-send-email-tgrabiec%40scylladb.com.

[Avi Kivity]

On 09/22/2016 03:55 PM, Nadav Har'El wrote:

Sorry about that, guys (Avi actually wrote this code initially, but I overhauled it and should have caught this).

I made a note to myself to explain in the Seastar tutorial why recursion is *not* a good replacement for Seastar's iteration primitives (do_until, repeat, etc.).

That being said, there is something I didn't fully understood here:
The code does something like this:

future<> consume() {

           return fd.get().then([] { return consume(); });
}

So if fd.get() is not ready, we don't have recursion here at all - consume() returns a future immediately, and sometime later will be called again (no recursion).

So recursion only happens if fd.get() is ready immediately.

But when this code is run immediately, then because of the "return" and the compiler could figure out that the code doesn't need any of the on-stack variables, the compiler could do "tail call optimization" to *replace* the last frame, instead of add to it, and avoid recursion. Why doesn't the compiler do it? Is it because C++ destruction order reasons (e.g., C++ thinks it needs to guarantee destruction of objects after then() returns, not before? Or just optimizer limitations?

Anyway, sure it's better to avoid relying on the tail call optimization...

It's only a tail call if "return consume()" is inlined all the way through then() into consume().  But if one of the intermediate calls doesn't get inlined, it's no longer a tail call.

I'm not even sure gcc replaces tail recursion by tail calls.

To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.

To post to this group, send email to seast...@googlegroups.com.
Visit this group at https://groups.google.com/group/seastar-dev.

To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/CANEVyjuz%3DCn57woeZ57YazEQv%2BBGwKCbCYwjcC0NJnnUcCq8ww%40mail.gmail.com.

[Tomasz Grabiec]

Yes. In this case, future::then() is not inlined into consume():

#1326 0x0000000000948a05 in future<>
input_stream<char>::consume<sstables::data_consume_rows_context>(sstables::data_consume_rows_context&)::{lambda(temporary_buffer<char>)#1}::operator()(temporary_buffer<char>)
const (buf=..., __closure=0x600010e03880)
at /home/tgrabiec/src/scylla/seastar/core/iostream-impl.hh:154
#1327 apply_helper<future<>
input_stream<char>::consume<sstables::data_consume_rows_context>(sstables::data_consume_rows_context&)::{lambda(temporary_buffer<char>)#1},
std::tuple<temporary_buffer<char> >&&, std::integer_sequence<unsigned
long, 0ul> >::apply({lambda(temporary_buffer<char>)#1}&&,
std::tuple<temporary_buffer<char> >) (args=<optimized out>,
func=<unknown type in
/home/tgrabiec/.dtest/dtest-Ty7G1t/test/node1/bin/scylla, CU
0x4a68d01, DIE 0x4b9a71d>)
at /home/tgrabiec/src/scylla/seastar/core/apply.hh:34
#1328 apply<future<>
input_stream<char>::consume<sstables::data_consume_rows_context>(sstables::data_consume_rows_context&)::{lambda(temporary_buffer<char>)#1},
temporary_buffer<char> >(sstables::data_consume_rows_context&&,
std::tuple<temporary_buffer<char> >&&)
(args=<optimized out>, func=<optimized out>) at
/home/tgrabiec/src/scylla/seastar/core/apply.hh:42
#1329 futurize<future<> >::apply<future<>
input_stream<char>::consume<sstables::data_consume_rows_context>(sstables::data_consume_rows_context&)::{lambda(temporary_buffer<char>)#1},
temporary_buffer<char> >(sstables::data_consume_rows_context&&,
std::tuple<temporary_buffer<char> >&&) (
func=<unknown type in
/home/tgrabiec/.dtest/dtest-Ty7G1t/test/node1/bin/scylla, CU
0x4a68d01, DIE 0x4b9a6d5>,
args=<optimized out>) at
/home/tgrabiec/src/scylla/seastar/core/future.hh:1234
#1330 0x0000000000948e99 in future<temporary_buffer<char>
>::then<future<>
input_stream<char>::consume<sstables::data_consume_rows_context>(sstables::data_consume_rows_context&)::{lambda(temporary_buffer<char>)#1},
future<> >(sstables::data_consume_rows_context&&)
(this=this@entry=0x600010e038b0,
func=func@entry=<unknown type in
/home/tgrabiec/.dtest/dtest-Ty7G1t/test/node1/bin/scylla, CU
0x4a68d01, DIE 0x4b9ac2e>)
at /home/tgrabiec/src/scylla/seastar/core/future.hh:860
#1331 0x0000000000948460 in
input_stream<char>::consume<sstables::data_consume_rows_context>
(this=0x6000002c7ec0, consumer=...)
at /home/tgrabiec/src/scylla/seastar/core/iostream-impl.hh:155
#1332 0x0000000000948a05 in future<>
input_stream<char>::consume<sstables::data_consume_rows_context>(sstables::data_consume_rows_context&)::{lambda(temporary_buffer<char>)#1}::operator()(temporary_buffer<char>)
const (buf=..., __closure=0x600010e03a90)
at /home/tgrabiec/src/scylla/seastar/core/iostream-impl.hh:154