[PATCH v2 00/18] Add support for Coroutines TS

[Paweł Dziepak]

Hi,

This series adds experimental support for Coroutines TS. It requires
clang and needs to be enabled with flag
--enable-experimental-coroutines-ts. Header file
seastar/coore/coroutines.hh provides all that is needed to start using
them and libc++ is not required.

Coroutines seamlessly integrate with seastar continuations, futures and
promises. Any function that returns a future can be made into a coroutine
and a coroutine can for any future. The last patch adds a demo that shows
some basic usage.

Known problems and limitations:
- there is an open todo in clang that prevents ubsan from working with
coroutines https://bugs.llvm.org/show_bug.cgi?id=36296
This patch for clang works around it: https://reviews.llvm.org/D44672
- resuming a coroutine involves double indirect call: first to virtual
function task::run_and_dispose() and then to actually resume the
coroutine, this could be improved by having tighter integration
between the reactor.cc and the coroutines, but considering their
experimental status it is probably too early for that

Still missing (but probably shouldn't delay this series):
- coroutines in the standard
- bug-free compilers

Tests: unit(release: gcc8 without coroutines, patched clang8 with
coroutines)

Also in:

https://github.com/pdziepak/seastar.git coroutines-ts/v2

in v2:
- rebased
- fixed clang build
- dropped task::dispose and instead an broken_promise is thrown if
a continuation, seastar thread of a coroutine are abandoned

Paweł Dziepak (18):
treewide: remove redundant or harmful moves
treewide: drop unneeded lambda captures
apps/io_tester: add virtual dtor for class_data
apps/io_tester: fix class/struct mismatch
tests/mock_file: add missing override
apps/iotune: drop struct from std::vector
treewide: drop unused private class member
tests/httpd: extra_big_object: don't ask for default move ctor
memory: use gnu::used instead of gnu::externally_visible
memory: use old-style attribute for lambda
reactor: use no_sanitize("undefined")
exception_hacks: use used instead of externally_visible
temporary_buffer: hide gcc pragmas from clang
future: propagate broken_promise exception to abandoned continuations
core: add std-coroutine.hh
add support for coroutines
tests: add coroutines test
demos: add coroutines demo

configure.py | 6 +
demos/tls_echo_server.hh | 2 +-
include/seastar/core/coroutine.hh | 173 ++++++++++++++++++
include/seastar/core/future.hh | 69 ++++++-
include/seastar/core/seastar.hh | 4 +-
include/seastar/core/shared_ptr.hh | 8 +-
include/seastar/core/std-coroutine.hh | 92 ++++++++++
include/seastar/core/temporary_buffer.hh | 4 +
include/seastar/http/httpd.hh | 2 +-
include/seastar/json/formatter.hh | 2 +-
include/seastar/net/packet.hh | 6 +-
include/seastar/net/tcp-stack.hh | 4 +-
include/seastar/net/tcp.hh | 2 +-
.../seastar/util/alloc_failure_injector.hh | 2 +-
tests/unit/loopback_socket.hh | 4 +-
tests/unit/mock_file.hh | 2 +-
apps/io_tester/io_tester.cc | 8 +-
apps/iotune/iotune.cc | 14 +-
apps/memcached/memcache.cc | 2 +-
demos/coroutines_demo.cc | 47 +++++
src/core/exception_hacks.cc | 4 +-
src/core/memory.cc | 9 +-
src/core/reactor.cc | 6 +-
src/http/transformers.cc | 4 +-
src/net/dns.cc | 3 +-
src/rpc/rpc.cc | 6 +-
tests/unit/coroutines_test.cc | 114 ++++++++++++
tests/unit/fair_queue_test.cc | 19 --
tests/unit/futures_test.cc | 40 +++-
tests/unit/httpd_test.cc | 30 ++-
CMakeLists.txt | 9 +
demos/CMakeLists.txt | 5 +
tests/unit/CMakeLists.txt | 5 +
33 files changed, 618 insertions(+), 89 deletions(-)
create mode 100644 include/seastar/core/coroutine.hh
create mode 100644 include/seastar/core/std-coroutine.hh
create mode 100644 demos/coroutines_demo.cc
create mode 100644 tests/unit/coroutines_test.cc

--
2.20.1

[Paweł Dziepak]

---
src/http/transformers.cc | 4 ++--
src/rpc/rpc.cc | 4 ++--
tests/unit/futures_test.cc | 2 +-
tests/unit/httpd_test.cc | 8 ++++----
4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/http/transformers.cc b/src/http/transformers.cc
index bf526a12..4f5178c1 100644
--- a/src/http/transformers.cc
+++ b/src/http/transformers.cc
@@ -245,7 +245,7 @@ temporary_buffer<char> buffer_replace::match(temporary_buffer<char>& buf) {
if (_current.last()) {
auto res = get_remaining();
_current.erase(first);
- return std::move(res);
+ return res;
}
first = _current.erase(first);
} else {
@@ -257,7 +257,7 @@ temporary_buffer<char> buffer_replace::match(temporary_buffer<char>& buf) {
temporary_buffer<char> res(value.data(), value.size());
buf.trim_front(len_compare);
_current.clear();
- return std::move(res);
+ return res;
}
// only partial match
pos += len_compare;
diff --git a/src/rpc/rpc.cc b/src/rpc/rpc.cc
index 5414b84d..94b5781e 100644
--- a/src/rpc/rpc.cc
+++ b/src/rpc/rpc.cc
@@ -66,9 +66,9 @@ namespace rpc {
buf = _compressor->compress(4, std::move(buf));
static_assert(snd_buf::chunk_size >= 4, "send buffer chunk size is too small");
write_le<uint32_t>(buf.front().get_write(), buf.size - 4);
- return std::move(buf);
+ return buf;
}
- return std::move(buf);
+ return buf;
}

future<> connection::send_buffer(snd_buf buf) {
diff --git a/tests/unit/futures_test.cc b/tests/unit/futures_test.cc
index 361780ca..b806f8d0 100644
--- a/tests/unit/futures_test.cc
+++ b/tests/unit/futures_test.cc
@@ -168,7 +168,7 @@ SEASTAR_TEST_CASE(test_failing_intermediate_promise_should_fail_the_master_futur
promise<> p1;
promise<> p2;

- auto f = p1.get_future().then([f = std::move(p2.get_future())] () mutable {
+ auto f = p1.get_future().then([f = p2.get_future()] () mutable {
return std::move(f);
}).then([] {
BOOST_REQUIRE(false);
diff --git a/tests/unit/httpd_test.cc b/tests/unit/httpd_test.cc
index 17a26c80..04fced35 100644
--- a/tests/unit/httpd_test.cc
+++ b/tests/unit/httpd_test.cc
@@ -394,8 +394,8 @@ class test_client_server {

auto client = seastar::async([&lsi, reader] {
connected_socket c_socket = std::get<connected_socket>(lsi.connect(socket_address(ipv4_addr()), socket_address(ipv4_addr())).get());
- input_stream<char> input(std::move(c_socket.input()));
- output_stream<char> output(std::move(c_socket.output()));
+ input_stream<char> input(c_socket.input());
+ output_stream<char> output(c_socket.output());
bool more = true;
size_t count = 0;
while (more) {
@@ -457,8 +457,8 @@ class test_client_server {

auto client = seastar::async([&lsi, tests] {
connected_socket c_socket = std::get<connected_socket>(lsi.connect(socket_address(ipv4_addr()), socket_address(ipv4_addr())).get());
- input_stream<char> input(std::move(c_socket.input()));
- output_stream<char> output(std::move(c_socket.output()));
+ input_stream<char> input(c_socket.input());
+ output_stream<char> output(c_socket.output());
bool more = true;
size_t count = 0;
while (more) {
--
2.20.1

[Paweł Dziepak]

---
demos/tls_echo_server.hh | 2 +-
tests/unit/loopback_socket.hh | 4 ++--
apps/io_tester/io_tester.cc | 4 ++--
apps/iotune/iotune.cc | 8 ++++----
src/core/reactor.cc | 4 ++--
src/rpc/rpc.cc | 2 +-
tests/unit/futures_test.cc | 4 ++--
tests/unit/httpd_test.cc | 18 +++++++++---------
8 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/demos/tls_echo_server.hh b/demos/tls_echo_server.hh
index 23d5ee35..d90ea2bc 100644
--- a/demos/tls_echo_server.hh
+++ b/demos/tls_echo_server.hh
@@ -91,7 +91,7 @@ class echoserver {
});
}).then([strms]{
return strms->out.close();
- }).handle_exception([this](auto ep) {
+ }).handle_exception([](auto ep) {
}).finally([this, strms]{
if (_verbose) {
std::cout << "Ending session" << std::endl;
diff --git a/tests/unit/loopback_socket.hh b/tests/unit/loopback_socket.hh
index 888b0692..efcd87e0 100644
--- a/tests/unit/loopback_socket.hh
+++ b/tests/unit/loopback_socket.hh
@@ -242,14 +242,14 @@ class loopback_socket_impl : public net::socket_impl {
return _factory.make_new_server_connection(std::move(b1), b2).then([b2] {
return make_foreign(b2);
});
- }).then([this, shard] (foreign_ptr<lw_shared_ptr<loopback_buffer>> b2) {
+ }).then([this] (foreign_ptr<lw_shared_ptr<loopback_buffer>> b2) {
return _factory.make_new_client_connection(_b1, std::move(b2));
});
}

void shutdown() {
_b1->shutdown();
- smp::submit_to(_b2.get_owner_shard(), [this, b2 = std::move(_b2)] {
+ smp::submit_to(_b2.get_owner_shard(), [b2 = std::move(_b2)] {
b2->shutdown();
});
}
diff --git a/apps/io_tester/io_tester.cc b/apps/io_tester/io_tester.cc
index 2c042709..0eddf167 100644
--- a/apps/io_tester/io_tester.cc
+++ b/apps/io_tester/io_tester.cc
@@ -150,7 +150,7 @@ class class_data {
return parallel_for_each(boost::irange(0u, parallelism()), [this, stop] (auto dummy) mutable {
auto bufptr = allocate_aligned_buffer<char>(this->req_size(), _alignment);
auto buf = bufptr.get();
- return do_until([this, stop] { return std::chrono::steady_clock::now() > stop; }, [this, buf, stop] () mutable {
+ return do_until([stop] { return std::chrono::steady_clock::now() > stop; }, [this, buf, stop] () mutable {
auto start = std::chrono::steady_clock::now();
return issue_request(buf).then([this, start, stop] (auto size) {
auto now = std::chrono::steady_clock::now();
@@ -297,7 +297,7 @@ class io_class_data : public class_data {
std::uniform_int_distribution<char> fill('@', '~');
memset(buf, fill(random_generator), bufsize);
pos = pos * bufsize;
- return _file.dma_write(pos, buf, bufsize).finally([this, bufsize, bufptr = std::move(bufptr), perm = std::move(perm), pos] {
+ return _file.dma_write(pos, buf, bufsize).finally([this, bufptr = std::move(bufptr), perm = std::move(perm), pos] {
if ((this->req_type() == request_type::append) && (pos > _last_pos)) {
_last_pos = pos;
}
diff --git a/apps/iotune/iotune.cc b/apps/iotune/iotune.cc
index f7eb2f04..0bea67bf 100644
--- a/apps/iotune/iotune.cc
+++ b/apps/iotune/iotune.cc
@@ -391,12 +391,12 @@ class test_file {

auto worker = worker_ptr.get();
auto concurrency = boost::irange<unsigned, unsigned>(0, max_os_concurrency, 1);
- return parallel_for_each(std::move(concurrency), [this, worker] (unsigned idx) {
+ return parallel_for_each(std::move(concurrency), [worker] (unsigned idx) {
auto bufptr = worker->get_buffer();
auto buf = bufptr.get();
- return do_until([worker] { return worker->should_stop(); }, [this, buf, worker, idx] {
+ return do_until([worker] { return worker->should_stop(); }, [buf, worker] {
return worker->issue_request(buf);
- }).finally([this, alive = std::move(bufptr)] {});
+ }).finally([alive = std::move(bufptr)] {});
}).then_wrapped([this, worker = std::move(worker_ptr), update_file_size] (future<> f) {
try {
f.get();
@@ -457,7 +457,7 @@ class iotune_multi_shard_context {
}

future<> create_data_file() {
- return _iotune_test_file.invoke_on_all([this] (test_file& tf) {
+ return _iotune_test_file.invoke_on_all([] (test_file& tf) {
return tf.create_data_file();
});
}
diff --git a/src/core/reactor.cc b/src/core/reactor.cc
index b9ab4652..c02b2640 100644
--- a/src/core/reactor.cc
+++ b/src/core/reactor.cc
@@ -165,7 +165,7 @@ reactor::update_shares_for_class(io_priority_class pc, uint32_t shares) {

future<pollable_fd, socket_address>
reactor::accept(pollable_fd_state& listenfd) {
- return readable_or_writeable(listenfd).then([this, &listenfd] () mutable {
+ return readable_or_writeable(listenfd).then([&listenfd] () mutable {
socket_address sa;
socklen_t sl = sizeof(&sa.u.sas);
file_desc fd = listenfd.fd.accept(sa.u.sa, sl, SOCK_NONBLOCK | SOCK_CLOEXEC);
@@ -5186,7 +5186,7 @@ void smp::configure(boost::program_options::variables_map configuration)
}
};

- auto assign_io_queue = [&ioq_topology, &all_io_queues, &disk_config] (shard_id shard_id, dev_t dev_id) {
+ auto assign_io_queue = [&ioq_topology, &all_io_queues] (shard_id shard_id, dev_t dev_id) {
auto io_info = ioq_topology.at(dev_id);
auto cid = io_info.shard_to_coordinator[shard_id];
auto queue_idx = io_info.coordinator_to_idx[cid];
diff --git a/src/rpc/rpc.cc b/src/rpc/rpc.cc
index 94b5781e..5e938ec6 100644
--- a/src/rpc/rpc.cc
+++ b/src/rpc/rpc.cc
@@ -448,7 +448,7 @@ namespace rpc {
}

return _stream_queue.not_empty().then([this, &bufs] {
- bool eof = !_stream_queue.consume([this, &bufs] (rcv_buf&& b) {
+ bool eof = !_stream_queue.consume([&bufs] (rcv_buf&& b) {
if (b.size == -1U) { // max fragment length marks an end of a stream
return false;
} else {
diff --git a/tests/unit/futures_test.cc b/tests/unit/futures_test.cc
index b806f8d0..3324f5a5 100644
--- a/tests/unit/futures_test.cc
+++ b/tests/unit/futures_test.cc
@@ -434,12 +434,12 @@ SEASTAR_TEST_CASE(test_parallel_for_each) {
BOOST_REQUIRE_EQUAL(sum, 15);

// throws immediately
- BOOST_CHECK_EXCEPTION(parallel_for_each(range, [&sum] (int) -> future<> {
+ BOOST_CHECK_EXCEPTION(parallel_for_each(range, [] (int) -> future<> {
throw 5;
}).get(), int, [] (int v) { return v == 5; });

// throws after suspension
- BOOST_CHECK_EXCEPTION(parallel_for_each(range, [&sum] (int) {
+ BOOST_CHECK_EXCEPTION(parallel_for_each(range, [] (int) {
return later().then([] {
throw 5;

});
diff --git a/tests/unit/httpd_test.cc b/tests/unit/httpd_test.cc

index 04fced35..a10a09bc 100644
--- a/tests/unit/httpd_test.cc
+++ b/tests/unit/httpd_test.cc
@@ -252,10 +252,10 @@ future<> test_transformer_stream(std::stringstream& ss, content_replace& cr, std
ss.str("");
req->_headers["Host"] = "localhost";
return do_with(output_stream<char>(cr.transform(std::move(req), "json", output_stream<char>(memory_data_sink(ss), 32000, true))),
- std::vector<sstring>(std::move(buffer_parts)), [&ss, &cr] (output_stream<char>& os, std::vector<sstring>& parts) {
+ std::vector<sstring>(std::move(buffer_parts)), [] (output_stream<char>& os, std::vector<sstring>& parts) {
return do_for_each(parts, [&os](auto& p) {
return os.write(p);
- }).then([&os, &ss] {
+ }).then([&os] {
return os.close();
});
});
@@ -264,7 +264,7 @@ future<> test_transformer_stream(std::stringstream& ss, content_replace& cr, std
SEASTAR_TEST_CASE(test_transformer) {
return do_with(std::stringstream(), content_replace("json"), [] (std::stringstream& ss, content_replace& cr) {
return do_with(output_stream<char>(cr.transform(std::make_unique<seastar::httpd::request>(), "html", output_stream<char>(memory_data_sink(ss), 32000, true))),
- [&ss] (output_stream<char>& os) {
+ [] (output_stream<char>& os) {
return os.write(sstring("hello-{{Protocol}}-xyz-{{Host}}")).then([&os] {
return os.close();
});
@@ -274,7 +274,7 @@ SEASTAR_TEST_CASE(test_transformer) {
BOOST_REQUIRE_EQUAL(ss.str(), "hello-http-xyz-localhost{{Pr");
return test_transformer_stream(ss, cr, {"hell", "o-{{", "Pro", "tocol}}{{Protocol}}-{{Protoxyz-{{Ho", "st}}{{Pr"}).then([&ss, &cr] {
BOOST_REQUIRE_EQUAL(ss.str(), "hello-httphttp-{{Protoxyz-localhost{{Pr");
- return test_transformer_stream(ss, cr, {"hell", "o-{{Pro", "t{{Protocol}}ocol}}", "{{Host}}"}).then([&ss, &cr] {
+ return test_transformer_stream(ss, cr, {"hell", "o-{{Pro", "t{{Protocol}}ocol}}", "{{Host}}"}).then([&ss] {
BOOST_REQUIRE_EQUAL(ss.str(), "hello-{{Prothttpocol}}localhost");
});
});
@@ -403,8 +403,8 @@ class test_client_server {
htp._concat = false;

write_request(output).get();
- repeat([&c_socket, &input, &htp] {
- return input.read().then([&c_socket, &input, &htp](const temporary_buffer<char>& b) mutable {
+ repeat([&input, &htp] {
+ return input.read().then([&htp](const temporary_buffer<char>& b) mutable {
return (b.size() == 0 || htp.read(b)) ? make_ready_future<stop_iteration>(stop_iteration::yes) :
make_ready_future<stop_iteration>(stop_iteration::no);
});
@@ -464,8 +464,8 @@ class test_client_server {
while (more) {
http_consumer htp;
write_request(output).get();
- repeat([&c_socket, &input, &htp] {
- return input.read().then([&c_socket, &input, &htp](const temporary_buffer<char>& b) mutable {
+ repeat([&input, &htp] {
+ return input.read().then([&htp](const temporary_buffer<char>& b) mutable {
return (b.size() == 0 || htp.read(b)) ? make_ready_future<stop_iteration>(stop_iteration::yes) :
make_ready_future<stop_iteration>(stop_iteration::no);
});
@@ -530,7 +530,7 @@ class test_client_server {
throw std::runtime_error("Throwing exception before writing");
}
}
- return repeat([&str, &remain, success] () mutable {
+ return repeat([&str, &remain] () mutable {
return str.write("1234567890").then([&remain]() mutable {
remain--;
return (remain == 0)? make_ready_future<stop_iteration>(stop_iteration::yes) : make_ready_future<stop_iteration>(stop_iteration::no);
--
2.20.1

[Paweł Dziepak]

---
apps/io_tester/io_tester.cc | 2 ++
1 file changed, 2 insertions(+)

diff --git a/apps/io_tester/io_tester.cc b/apps/io_tester/io_tester.cc
index 0eddf167..1c36c0ee 100644
--- a/apps/io_tester/io_tester.cc
+++ b/apps/io_tester/io_tester.cc
@@ -144,6 +144,8 @@ class class_data {
, _pos_distribution(0, file_data_size / _config.shard_info.request_size)
{}

+ virtual ~class_data() = default;
+
future<> issue_requests(std::chrono::steady_clock::time_point stop) {
_start = std::chrono::steady_clock::now();
return with_scheduling_group(_sg, [this, stop] {
--
2.20.1

[Paweł Dziepak]

---
include/seastar/core/future.hh | 2 +-
include/seastar/core/seastar.hh | 4 ++--
include/seastar/core/shared_ptr.hh | 8 ++++----

include/seastar/http/httpd.hh | 2 +-
include/seastar/json/formatter.hh | 2 +-

include/seastar/net/packet.hh | 6 +++---
include/seastar/net/tcp-stack.hh | 4 ++--
include/seastar/net/tcp.hh | 2 +-
include/seastar/util/alloc_failure_injector.hh | 2 +-
apps/io_tester/io_tester.cc | 2 +-
apps/memcached/memcache.cc | 2 +-
11 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh
index 15e19110..e39cd41e 100644
--- a/include/seastar/core/future.hh
+++ b/include/seastar/core/future.hh
@@ -564,7 +564,7 @@ class promise {
template <typename... U>
friend class future;

- friend class future_state<T...>;
+ friend struct future_state<T...>;
};

/// \brief Specialization of \c promise<void>
diff --git a/include/seastar/core/seastar.hh b/include/seastar/core/seastar.hh
index 3c80355a..25f893cf 100644
--- a/include/seastar/core/seastar.hh
+++ b/include/seastar/core/seastar.hh
@@ -54,12 +54,12 @@ template <class CharType> class output_stream;
class server_socket;
class connected_socket;
class socket_address;
-class listen_options;
+struct listen_options;
enum class transport;

// file.hh
class file;
-class file_open_options;
+struct file_open_options;
enum class open_flags;
enum class fs_type;

diff --git a/include/seastar/core/shared_ptr.hh b/include/seastar/core/shared_ptr.hh
index 0114198a..3ed3b0ce 100644
--- a/include/seastar/core/shared_ptr.hh
+++ b/include/seastar/core/shared_ptr.hh
@@ -148,9 +148,9 @@ class enable_lw_shared_from_this : private lw_shared_ptr_counter_base {
template <typename X>
friend class lw_shared_ptr;
template <typename X>
- friend class internal::lw_shared_ptr_accessors_esft;
+ friend struct internal::lw_shared_ptr_accessors_esft;
template <typename X, class Y>
- friend class internal::lw_shared_ptr_accessors;
+ friend struct internal::lw_shared_ptr_accessors;
};

template <typename T>
@@ -166,9 +166,9 @@ struct shared_ptr_no_esft : private lw_shared_ptr_counter_base {
template <typename X>
friend class lw_shared_ptr;
template <typename X>
- friend class internal::lw_shared_ptr_accessors_no_esft;
+ friend struct internal::lw_shared_ptr_accessors_no_esft;
template <typename X, class Y>
- friend class internal::lw_shared_ptr_accessors;
+ friend struct internal::lw_shared_ptr_accessors;
};


diff --git a/include/seastar/http/httpd.hh b/include/seastar/http/httpd.hh
index 102e17e3..e0ce941e 100644
--- a/include/seastar/http/httpd.hh
+++ b/include/seastar/http/httpd.hh
@@ -49,7 +49,7 @@ namespace httpd {

class http_server;
class http_stats;
-class reply;
+struct reply;

using namespace std::chrono_literals;

diff --git a/include/seastar/json/formatter.hh b/include/seastar/json/formatter.hh
index dd1fc87e..39b087c2 100644
--- a/include/seastar/json/formatter.hh
+++ b/include/seastar/json/formatter.hh
@@ -34,7 +34,7 @@ namespace seastar {

namespace json {

-struct jsonable;
+class jsonable;

typedef struct tm date_time;

diff --git a/include/seastar/net/packet.hh b/include/seastar/net/packet.hh
index 18e8eb39..134e4bfc 100644
--- a/include/seastar/net/packet.hh
+++ b/include/seastar/net/packet.hh
@@ -311,9 +311,9 @@ class packet final {
void linearize(size_t at_frag, size_t desired_size);
bool allocate_headroom(size_t size);
public:
- class offload_info offload_info() const { return _impl->_offload_info; }
- class offload_info& offload_info_ref() { return _impl->_offload_info; }
- void set_offload_info(class offload_info oi) { _impl->_offload_info = oi; }
+ struct offload_info offload_info() const { return _impl->_offload_info; }
+ struct offload_info& offload_info_ref() { return _impl->_offload_info; }
+ void set_offload_info(struct offload_info oi) { _impl->_offload_info = oi; }
};

std::ostream& operator<<(std::ostream& os, const packet& p);
diff --git a/include/seastar/net/tcp-stack.hh b/include/seastar/net/tcp-stack.hh
index 0deca237..d5766d9c 100644
--- a/include/seastar/net/tcp-stack.hh
+++ b/include/seastar/net/tcp-stack.hh
@@ -27,13 +27,13 @@

namespace seastar {

-class listen_options;
+struct listen_options;
class server_socket;
class connected_socket;

namespace net {

-class ipv4_traits;
+struct ipv4_traits;
template <typename InetTraits>
class tcp;

diff --git a/include/seastar/net/tcp.hh b/include/seastar/net/tcp.hh
index 4b9ca96a..a4755782 100644
--- a/include/seastar/net/tcp.hh
+++ b/include/seastar/net/tcp.hh
@@ -50,7 +50,7 @@ using namespace std::chrono_literals;

namespace net {

-class tcp_hdr;
+struct tcp_hdr;

inline auto tcp_error(int err) {
return std::system_error(err, std::system_category());
diff --git a/include/seastar/util/alloc_failure_injector.hh b/include/seastar/util/alloc_failure_injector.hh
index 908f9c0e..6853acb4 100644
--- a/include/seastar/util/alloc_failure_injector.hh
+++ b/include/seastar/util/alloc_failure_injector.hh
@@ -53,7 +53,7 @@ class alloc_failure_injector {
noncopyable_function<void()> _on_alloc_failure = [] { throw std::bad_alloc(); };
bool _failed;
uint64_t _suppressed = 0;
- friend class disable_failure_guard;
+ friend struct disable_failure_guard;
private:
void fail();
public:
diff --git a/apps/io_tester/io_tester.cc b/apps/io_tester/io_tester.cc
index 1c36c0ee..5fd123f8 100644
--- a/apps/io_tester/io_tester.cc
+++ b/apps/io_tester/io_tester.cc
@@ -57,7 +57,7 @@ static std::default_random_engine random_generator(random_seed);
// that will push the data out of the disk's cache. And static sizes per file are simpler.
static constexpr uint64_t file_data_size = 1ull << 30;

-struct context;
+class context;
enum class request_type { seqread, seqwrite, randread, randwrite, append, cpu };

namespace std {
diff --git a/apps/memcached/memcache.cc b/apps/memcached/memcache.cc
index 747ec5bd..0be3b8c7 100644
--- a/apps/memcached/memcache.cc
+++ b/apps/memcached/memcache.cc
@@ -269,7 +269,7 @@ class item : public slab_item_base {
assert(it->_ref_count >= 0);
}

- friend class item_key_cmp;
+ friend struct item_key_cmp;
};

struct item_key_cmp
--
2.20.1

[Paweł Dziepak]

---
tests/unit/mock_file.hh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/unit/mock_file.hh b/tests/unit/mock_file.hh
index 3ee47695..fcefc60e 100644
--- a/tests/unit/mock_file.hh
+++ b/tests/unit/mock_file.hh
@@ -84,7 +84,7 @@ class mock_read_only_file final : public file_impl {
virtual future<struct stat> stat() override {
throw std::bad_function_call();
}
- virtual future<> truncate(uint64_t) {
+ virtual future<> truncate(uint64_t) override {
throw std::bad_function_call();
}
virtual future<> discard(uint64_t offset, uint64_t length) override {
--
2.20.1

[Paweł Dziepak]

---
apps/iotune/iotune.cc | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/apps/iotune/iotune.cc b/apps/iotune/iotune.cc
index 0bea67bf..1d727952 100644
--- a/apps/iotune/iotune.cc
+++ b/apps/iotune/iotune.cc
@@ -517,7 +517,7 @@ void write_configuration_file(sstring conf_file, std::string format, sstring pro
string_to_file(conf_file, buf);
}

-void write_property_file(sstring conf_file, struct std::vector<disk_descriptor> disk_descriptors) {
+void write_property_file(sstring conf_file, std::vector<disk_descriptor> disk_descriptors) {
YAML::Emitter out;
out << YAML::BeginMap;
out << YAML::Key << "disks";
@@ -583,7 +583,7 @@ int main(int ac, char** av) {
auto format = configuration["format"].as<sstring>();
auto duration = std::chrono::duration<double>(configuration["duration"].as<unsigned>() * 1s);

- struct std::vector<disk_descriptor> disk_descriptors;
+ std::vector<disk_descriptor> disk_descriptors;
std::unordered_map<sstring, sstring> mountpoint_map;
// We want to evaluate once per mountpoint, but we still want to write in one of the
// directories that we were provided - we may not have permissions to write into the
--
2.20.1

[Paweł Dziepak]

---
apps/iotune/iotune.cc | 2 --
src/net/dns.cc | 3 +--
tests/unit/fair_queue_test.cc | 19 -------------------
3 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/apps/iotune/iotune.cc b/apps/iotune/iotune.cc
index 1d727952..b15ef62c 100644
--- a/apps/iotune/iotune.cc
+++ b/apps/iotune/iotune.cc
@@ -286,7 +286,6 @@ class io_worker {
uint64_t _bytes = 0;
unsigned _requests = 0;
size_t _buffer_size;
- std::chrono::duration<double> _duration;
std::chrono::time_point<iotune_clock, std::chrono::duration<double>> _start_measuring;
std::chrono::time_point<iotune_clock, std::chrono::duration<double>> _end_measuring;
std::chrono::time_point<iotune_clock, std::chrono::duration<double>> _end_load;
@@ -306,7 +305,6 @@ class io_worker {

io_worker(size_t buffer_size, std::chrono::duration<double> duration, std::unique_ptr<request_issuer> reqs, std::unique_ptr<position_generator> pos)
: _buffer_size(buffer_size)
- , _duration(duration)
, _start_measuring(iotune_clock::now() + std::chrono::duration<double>(10ms))
, _end_measuring(_start_measuring + duration)
, _end_load(_end_measuring + 10ms)
diff --git a/src/net/dns.cc b/src/net/dns.cc
index 3833a4c4..a9a9b2a7 100644
--- a/src/net/dns.cc
+++ b/src/net/dns.cc
@@ -922,7 +922,7 @@ class net::dns_resolver::impl
network_stack & _stack;

ares_channel _channel = {};
- uint64_t _ops = 0, _calls = 0;
+ uint64_t _calls = 0;
std::chrono::milliseconds _timeout;
timer<> _timer;
gate _gate;
@@ -1036,4 +1036,3 @@ future<std::vector<net::inet_address>> net::inet_address::find_all(
}

}
-
diff --git a/tests/unit/fair_queue_test.cc b/tests/unit/fair_queue_test.cc
index 68eec3cd..b2743cef 100644
--- a/tests/unit/fair_queue_test.cc
+++ b/tests/unit/fair_queue_test.cc
@@ -37,25 +37,6 @@
using namespace seastar;
using namespace std::chrono_literals;

-class test_request {
- fair_queue* _fq;
- promise<> _pr;
- future<> _res;
-public:
- test_request(fair_queue& fq) : _fq(&fq), _res(make_exception_future<>(std::runtime_error("impossible"))) {}
- ~test_request() {
- }
-
- test_request(const test_request&) = delete;
- test_request(test_request&&) = default;
- future<> get_future() {
- return _pr.get_future();
- }
- void add_result(future<> f) {
- _res = std::move(f);
- }
-};
-
fair_queue::config make_config(unsigned capacity) {
fair_queue::config cfg;
cfg.capacity = capacity;
--
2.20.1

[Paweł Dziepak]

---
tests/unit/httpd_test.cc | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/unit/httpd_test.cc b/tests/unit/httpd_test.cc
index a10a09bc..aeeaa304 100644
--- a/tests/unit/httpd_test.cc
+++ b/tests/unit/httpd_test.cc
@@ -623,8 +623,6 @@ struct extra_big_object : public json::json_base {
_elements.emplace_back(value);
}
}
-
- extra_big_object(extra_big_object&&) = default;
};

SEASTAR_TEST_CASE(json_stream) {
@@ -632,7 +630,7 @@ SEASTAR_TEST_CASE(json_stream) {
size_t num_objects = 1000;
size_t total_size = num_objects * 1000001 + 1;
for (size_t i = 0; i < num_objects; i++) {
- vec.emplace_back(extra_big_object(1000000));
+ vec.emplace_back(1000000);
}
return test_client_server::run_test(json::stream_object(vec), [total_size](size_t s, http_consumer& h) {
BOOST_REQUIRE_EQUAL(h._size, total_size);
--
2.20.1

[Paweł Dziepak]

gnu::used seems to be a better choice than gnu::externally_visible to
guarantee that the function will actually exist since it is unaffected by
'inline' and supported by clang.
---
src/core/memory.cc | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/core/memory.cc b/src/core/memory.cc
index a4f8ad8b..12f4690d 100644
--- a/src/core/memory.cc
+++ b/src/core/memory.cc
@@ -1496,7 +1496,7 @@ using namespace seastar::memory;

extern "C"
[[gnu::visibility("default")]]
-[[gnu::externally_visible]]
+[[gnu::used]]
void* malloc(size_t n) throw () {
if (try_trigger_error_injector()) {
return nullptr;
@@ -1511,7 +1511,7 @@ void* __libc_malloc(size_t n) throw ();

extern "C"
[[gnu::visibility("default")]]
-[[gnu::externally_visible]]
+[[gnu::used]]
void free(void* ptr) {
if (ptr) {
seastar::memory::free(ptr);
@@ -1580,7 +1580,7 @@ void* __libc_realloc(void* obj, size_t size) throw ();

extern "C"
[[gnu::visibility("default")]]
-[[gnu::externally_visible]]
+[[gnu::used]]
int posix_memalign(void** ptr, size_t align, size_t size) {
if (try_trigger_error_injector()) {
return ENOMEM;
@@ -1899,4 +1899,3 @@ namespace seastar {
/// \endcond

}
-
--
2.20.1

[Paweł Dziepak]

C++11 attributes apply to lambda's operator() function type and not to the
function itself. This means that, for example, [[noreturn]] cannot be used
in such context. When it comes to compiler-specific attributes GCC appears
to not care about this but clang does. Using the old-style __attribute__
appeases both compilers.
---
src/core/memory.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/memory.cc b/src/core/memory.cc
index 12f4690d..6b8dea2b 100644
--- a/src/core/memory.cc
+++ b/src/core/memory.cc
@@ -1239,7 +1239,7 @@ static inline cpu_pages& get_cpu_mem()
if (__builtin_expect(!bool(cpu_mem_ptr), false)) {
// Mark as cold so that GCC8+ can move this part of the function
// to .text.unlikely.
- [&] () [[gnu::cold]] {
+ [&] () __attribute__((cold)) {
cpu_mem_ptr = &cpu_mem;
}();
}
--
2.20.1

[Paweł Dziepak]

[[gnu::no_sanitize_undefined]] is not supported by clang.
---
src/core/reactor.cc | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/reactor.cc b/src/core/reactor.cc
index c02b2640..77f6ce0f 100644
--- a/src/core/reactor.cc
+++ b/src/core/reactor.cc
@@ -1149,7 +1149,7 @@ reactor::task_queue::task_queue(unsigned id, sstring name, float shares)
});
}

-[[gnu::no_sanitize_undefined]] // multiplication below may overflow; we check for that
+__attribute__((no_sanitize("undefined"))) // multiplication below may overflow; we check for that
inline
int64_t
reactor::task_queue::to_vruntime(sched_clock::duration runtime) const {
--
2.20.1

[Paweł Dziepak]

gnu::externally_visible is not supported by clang. It is also weaker than
gnu::used since it doesn't forces inlined functions to be emitted.
---
src/core/exception_hacks.cc | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/core/exception_hacks.cc b/src/core/exception_hacks.cc
index c5c8680e..547821e1 100644
--- a/src/core/exception_hacks.cc
+++ b/src/core/exception_hacks.cc
@@ -106,7 +106,7 @@ void log_exception_trace() noexcept {}
#ifndef SEASTAR_NO_EXCEPTION_HACK

extern "C"
[[gnu::visibility("default")]]
-[[gnu::externally_visible]]
+[[gnu::used]]

int dl_iterate_phdr(int (*callback) (struct dl_phdr_info *info, size_t size, void *data), void *data) {
if (!seastar::local_engine || !seastar::phdrs_cache.size()) {
// Cache is not yet populated, pass through to original function
@@ -130,7 +130,7 @@ int dl_iterate_phdr(int (*callback) (struct dl_phdr_info *info, size_t size, voi
#ifndef NO_EXCEPTION_INTERCEPT

extern "C"
[[gnu::visibility("default")]]
-[[gnu::externally_visible]]
+[[gnu::used]]

int _Unwind_RaiseException(struct _Unwind_Exception *h) {
using throw_fn = int (*)(void *);
static throw_fn org = nullptr;
--
2.20.1

[Paweł Dziepak]

Unsurprisingly, clang has issues with gcc-specific pargmas.
---
include/seastar/core/temporary_buffer.hh | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/include/seastar/core/temporary_buffer.hh b/include/seastar/core/temporary_buffer.hh
index 5ff4d680..48180def 100644
--- a/include/seastar/core/temporary_buffer.hh
+++ b/include/seastar/core/temporary_buffer.hh
@@ -82,14 +82,18 @@ class temporary_buffer {
temporary_buffer(const temporary_buffer&) = delete;

// At least at -O1, the inline decisions are such that the following code hits https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88897.
+#ifndef __clang__
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+#endif
/// Moves a \c temporary_buffer.
temporary_buffer(temporary_buffer&& x) noexcept : _buffer(x._buffer), _size(x._size), _deleter(std::move(x._deleter)) {
x._buffer = nullptr;
x._size = 0;
}
+#ifndef __clang__
#pragma GCC diagnostic pop
+#endif

/// Creates a \c temporary_buffer with a specific deleter.
///
--
2.20.1

[Paweł Dziepak]

---
include/seastar/core/future.hh | 49 +++++++++++++++++++++++++++++++++-
tests/unit/futures_test.cc | 34 +++++++++++++++++++++++
2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh
index e39cd41e..67b7486a 100644
--- a/include/seastar/core/future.hh
+++ b/include/seastar/core/future.hh
@@ -103,6 +103,16 @@ void engine_exit(std::exception_ptr eptr = {});
void report_failed_future(std::exception_ptr ex);
/// \endcond

+/// \brief Exception type for broken promises
+///
+/// When a promise is broken, i.e. a promise object with an attached
+/// continuation is destroyed before setting any value or exception, an
+/// exception of `broken_promise` type is propagated to that abandoned
+/// continuation.
+struct broken_promise : std::logic_error {
+ broken_promise() : logic_error("broken promise") { }
+};
+
//
// A future/promise pair maintain one logical value (a future_state).
// To minimize allocations, the value is stored in exactly one of three
@@ -759,7 +769,19 @@ class future {
}
template <typename Func>
void schedule(Func&& func) {
- if (state()->available()) {
+ if (state()->available() || !_promise) {
+ if (__builtin_expect(!state()->available() && !_promise, false)) {
+ // Encourage the compiler to move this away from the hot paths. __builtin_expect is not enough
+ // to do that. Cold lambdas work (at least for GCC8+).
+ [&] () __attribute__((cold)) {
+ try {
+ // Constructing broken_promise may throw (std::logic_error ctor is not noexcept).
+ _local_state.set_exception(std::make_exception_ptr(broken_promise{}));
+ } catch (...) {
+ _local_state.set_exception(std::current_exception());
+ }
+ }();
+ }
::seastar::schedule(std::make_unique<continuation<Func, T...>>(std::move(func), std::move(*state())));
} else {
assert(_promise);
@@ -899,6 +921,19 @@ class future {
}
};
void do_wait() noexcept {
+ if (__builtin_expect(!_promise, false)) {
+ // Encourage the compiler to move this away from the hot paths. __builtin_expect is not enough
+ // to do that. Cold lambdas work (at least for GCC8+).
+ [&] () __attribute__((cold)) {
+ try {
+ // Constructing broken_promise may throw (std::logic_error ctor is not noexcept).
+ _local_state.set_exception(std::make_exception_ptr(broken_promise{}));
+ } catch (...) {
+ _local_state.set_exception(std::current_exception());
+ }
+ }();
+ return;
+ }
auto thread = thread_impl::get();
assert(thread);
thread_wake_task wake_task{thread, this};
@@ -1268,6 +1303,18 @@ void promise<T...>::abandoned() noexcept {
_future->_promise = nullptr;
} else if (_state && _state->failed()) {
report_failed_future(_state->get_exception());
+ } else if (__builtin_expect(bool(_task), false)) {
+ assert(_state && !_state->available());
+ // Encourage the compiler to move this away from the hot paths. __builtin_expect is not enough
+ // to do that. Cold lambdas work (at least for GCC8+).
+ [&] () __attribute__((cold)) {
+ try {
+ // Constructing broken_promise may throw (std::logic_error ctor is not noexcept).
+ set_exception(broken_promise{});
+ } catch (...) {
+ set_exception(std::current_exception());
+ }
+ }();
}
}

diff --git a/tests/unit/futures_test.cc b/tests/unit/futures_test.cc
index 3324f5a5..17ffc509 100644
--- a/tests/unit/futures_test.cc
+++ b/tests/unit/futures_test.cc
@@ -29,6 +29,7 @@
#include <seastar/core/thread.hh>
#include <seastar/core/print.hh>
#include <boost/iterator/counting_iterator.hpp>
+#include <seastar/testing/thread_test_case.hh>

using namespace seastar;
using namespace std::chrono_literals;

@@ -954,3 +955,36 @@ SEASTAR_TEST_CASE(test_futurize_mutable) {
return seastar::stop_iteration::no;
});
}
+
+SEASTAR_THREAD_TEST_CASE(test_broken_promises) {
+ compat::optional<future<>> f;
+ compat::optional<future<>> f2;
+ { // Broken after attaching a continuation
+ auto p = promise<>();
+ f = p.get_future();
+ f2 = f->then_wrapped([&] (future<> f3) {
+ BOOST_CHECK(f3.failed());
+ BOOST_CHECK_THROW(f3.get(), broken_promise);
+ f = { };
+ });
+ }
+ f2->get();
+ BOOST_CHECK(!f);
+
+ { // Broken before attaching a continuation
+ auto p = promise<>();
+ f = p.get_future();
+ }
+ f->then_wrapped([&] (future<> f3) {
+ BOOST_CHECK(f3.failed());
+ BOOST_CHECK_THROW(f3.get(), broken_promise);
+ f = { };
+ }).get();
+ BOOST_CHECK(!f);
+
+ { // Broken before suspending a thread
+ auto p = promise<>();
+ f = p.get_future();
+ }
+ BOOST_CHECK_THROW(f->get(), broken_promise);
+}
--
2.20.1

[Paweł Dziepak]

std-coroutine.hh contains the necessary parts of <experimental/coroutine>
which are added so that the coroutines can be used even if the standard
library implementation doesn't provide that header (e.g. libstdc++).
---
include/seastar/core/std-coroutine.hh | 92 +++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
create mode 100644 include/seastar/core/std-coroutine.hh

diff --git a/include/seastar/core/std-coroutine.hh b/include/seastar/core/std-coroutine.hh
new file mode 100644
index 00000000..6707d47c
--- /dev/null
+++ b/include/seastar/core/std-coroutine.hh
@@ -0,0 +1,92 @@
+/*
+ * This file is open source software, licensed to you under the terms
+ * of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+ * distributed with this work for additional information regarding copyright
+ * ownership. You may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/*
+ * Copyright (C) 2019 ScyllaDB Ltd.
+ */
+
+#pragma once
+
+#if !SEASTAR_COROUTINES_TS
+#error Coroutines TS support disabled.
+#endif
+
+#if __has_include(<experimental/coroutine>)
+#include <experimental/coroutine>
+#else
+
+// We are not exactly allowed to defined anything in the std namespace, but this
+// makes coroutines work with libstdc++. All of this is experimental anyway.
+
+namespace std::experimental {
+
+template<typename Promise>
+class coroutine_handle {
+ void* _pointer = nullptr;
+public:
+ coroutine_handle() = default;
+
+ coroutine_handle &operator=(nullptr_t) noexcept {
+ _pointer = nullptr;
+ return *this;
+ }
+
+ explicit operator bool() const noexcept { return _pointer; }
+
+ static coroutine_handle from_address(void* ptr) noexcept {
+ coroutine_handle hndl;
+ hndl._pointer =ptr;
+ return hndl;
+ }
+ void* address() const noexcept { return _pointer; }
+
+ static coroutine_handle from_promise(Promise& promise) noexcept {
+ coroutine_handle hndl;
+ hndl._pointer = __builtin_coro_promise(&promise, alignof(Promise), true);
+ return hndl;
+ }
+ Promise& promise() const noexcept {
+ return *reinterpret_cast<Promise*>(__builtin_coro_promise(_pointer, alignof(Promise), false));
+ }
+
+ void operator()() noexcept { resume(); }
+
+ void resume() const noexcept { __builtin_coro_resume(_pointer); }
+ void destroy() const noexcept { __builtin_coro_destroy(_pointer); }
+ bool done() const noexcept { return __builtin_coro_done(_pointer); }
+};
+
+struct suspend_never {
+ constexpr bool await_ready() const noexcept { return true; }
+ template<typename T>
+ constexpr void await_suspend(coroutine_handle<T>) noexcept { }
+ constexpr void await_resume() noexcept { }
+};
+
+struct suspend_always {
+ constexpr bool await_ready() const noexcept { return false; }
+ template<typename T>
+ constexpr void await_suspend(coroutine_handle<T>) noexcept { }
+ constexpr void await_resume() noexcept { }
+};
+
+template<typename T, typename... Args>
+class coroutine_traits { };
+
+}
+
+#endif
--
2.20.1

[Paweł Dziepak]

This patch adds all necessary glue to make coroutines work with seastar
futures and promises. They integrate seamlessly and any function that
returns a future may become a coroutine.

seastar/core/coroutines.hh also defines some classes that should be in
experimental/coroutine (unless that file exists, in which case it is
included instead) so that this can work with libstdc++ which doesn't
provide that header.
---
configure.py | 6 ++
include/seastar/core/coroutine.hh | 173 ++++++++++++++++++++++++++++++
include/seastar/core/future.hh | 18 +++-
CMakeLists.txt | 9 ++
4 files changed, 205 insertions(+), 1 deletion(-)
create mode 100644 include/seastar/core/coroutine.hh

diff --git a/configure.py b/configure.py
index 9b70ef46..01ba36b9 100755
--- a/configure.py
+++ b/configure.py
@@ -96,6 +96,11 @@ add_tristate(
name = 'exception-scalability-workaround',
dest = 'exception_workaround',
help = 'a workaround for C++ exception scalability issues by overriding the definition of `dl_iterate_phdr`')
+add_tristate(
+ arg_parser,
+ name = 'experimental-coroutines-ts',
+ dest = "coroutines_ts",
+ help = 'experimental support for Coroutines TS')
arg_parser.add_argument('--allocator-page-size', dest='allocator_page_size', type=int, help='override allocator page size')
arg_parser.add_argument('--without-tests', dest='exclude_tests', action='store_true', help='Do not build tests by default')
arg_parser.add_argument('--without-apps', dest='exclude_apps', action='store_true', help='Do not build applications by default')
@@ -158,6 +163,7 @@ def configure_mode(mode):
tr(args.exception_workaround, 'EXCEPTION_SCALABILITY_WORKAROUND', value_when_none='yes'),
tr(args.allocator_page_size, 'ALLOCATOR_PAGE_SIZE'),
tr(args.cpp17_goodies, 'STD_OPTIONAL_VARIANT_STRINGVIEW'),
+ tr(args.coroutines_ts, 'EXPERIMENTAL_COROUTINES_TS'),
]

# Generate a new build by pointing to the source directory.
diff --git a/include/seastar/core/coroutine.hh b/include/seastar/core/coroutine.hh
new file mode 100644
index 00000000..8afb135f
--- /dev/null
+++ b/include/seastar/core/coroutine.hh
@@ -0,0 +1,173 @@

+/*
+ * This file is open source software, licensed to you under the terms
+ * of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+ * distributed with this work for additional information regarding copyright
+ * ownership. You may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/*
+ * Copyright (C) 2019 ScyllaDB Ltd.
+ */
+
+#pragma once
+

+#include <seastar/core/future.hh>

+
+#if !SEASTAR_COROUTINES_TS
+#error Coroutines TS support disabled.
+#endif
+

+#include <seastar/core/std-coroutine.hh>

+
+namespace std::experimental {
+

+template<typename... T, typename... Args>
+class coroutine_traits<seastar::future<T...>, Args...> {
+public:
+ class promise_type final : public seastar::task {
+ seastar::promise<T...> _promise;
+ public:
+ promise_type() = default;
+ promise_type(promise_type&&) = delete;
+ promise_type(const promise_type&) = delete;
+
+ template<typename... U>
+ void return_value(U&&... value) {
+ _promise.set_value(std::forward<U>(value)...);
+ }
+ void unhandled_exception() noexcept {
+ _promise.set_exception(std::current_exception());
+ }
+
+ seastar::future<T...> get_return_object() noexcept {
+ return _promise.get_future();
+ }
+
+ suspend_never initial_suspend() noexcept { return { }; }
+ suspend_never final_suspend() noexcept { return { }; }
+
+ virtual void run_and_dispose() noexcept override {
+ auto handle = std::experimental::coroutine_handle<promise_type>::from_promise(*this);
+ handle.resume();
+ }
+ };
+};
+
+template<typename... Args>
+class coroutine_traits<seastar::future<>, Args...> {
+public:
+ class promise_type final : public seastar::task {
+ seastar::promise<> _promise;
+ public:
+ promise_type() = default;
+ promise_type(promise_type&&) = delete;
+ promise_type(const promise_type&) = delete;
+
+ void return_void() noexcept {
+ _promise.set_value();
+ }
+ void unhandled_exception() noexcept {
+ _promise.set_exception(std::current_exception());
+ }
+
+ seastar::future<> get_return_object() noexcept {
+ return _promise.get_future();
+ }
+
+ suspend_never initial_suspend() noexcept { return { }; }
+ suspend_never final_suspend() noexcept { return { }; }
+
+ virtual void run_and_dispose() noexcept override {
+ auto handle = std::experimental::coroutine_handle<promise_type>::from_promise(*this);
+ handle.resume();
+ }
+ };
+};
+
+}
+
+namespace seastar {
+
+namespace internal {
+
+template<typename... T>
+struct awaiter {
+ seastar::future<T...> _future;
+public:
+ explicit awaiter(seastar::future<T...>&& f) noexcept : _future(std::move(f)) { }
+
+ awaiter(const awaiter&) = delete;
+ awaiter(awaiter&&) = delete;
+
+ bool await_ready() const noexcept {
+ return _future.available();
+ }
+
+ template<typename U>
+ void await_suspend(std::experimental::coroutine_handle<U> hndl) noexcept {
+ _future.set_coroutine(hndl.promise());
+ }
+
+ std::tuple<T...> await_resume() { return _future.get(); }
+};
+
+template<typename T>
+struct awaiter<T> {
+ seastar::future<T> _future;
+public:
+ explicit awaiter(seastar::future<T>&& f) noexcept : _future(std::move(f)) { }
+
+ awaiter(const awaiter&) = delete;
+ awaiter(awaiter&&) = delete;
+
+ bool await_ready() const noexcept {
+ return _future.available();
+ }
+
+ template<typename U>
+ void await_suspend(std::experimental::coroutine_handle<U> hndl) noexcept {
+ _future.set_coroutine(hndl.promise());
+ }
+
+ T await_resume() { return _future.get0(); }
+};
+
+template<>
+struct awaiter<> {
+ seastar::future<> _future;
+public:
+ explicit awaiter(seastar::future<>&& f) noexcept : _future(std::move(f)) { }
+
+ awaiter(const awaiter&) = delete;
+ awaiter(awaiter&&) = delete;
+
+ bool await_ready() const noexcept {
+ return _future.available();
+ }
+
+ template<typename U>
+ void await_suspend(std::experimental::coroutine_handle<U> hndl) noexcept {
+ _future.set_coroutine(hndl.promise());
+ }
+
+ void await_resume() { _future.get(); }
+};
+
+}
+
+template<typename... T>
+auto operator co_await(future<T...> f) noexcept {
+ return internal::awaiter<T...>(std::move(f));
+}
+
+}
diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh
index 67b7486a..4b53c61c 100644
--- a/include/seastar/core/future.hh
+++ b/include/seastar/core/future.hh
@@ -445,7 +445,7 @@ class promise {
future<T...>* _future = nullptr;
future_state<T...> _local_state;
future_state<T...>* _state;
- std::unique_ptr<continuation_base<T...>> _task;
+ std::unique_ptr<task> _task;
static constexpr bool copy_noexcept = future_state<T...>::copy_noexcept;
public:
/// \brief Constructs an empty \c promise.
@@ -528,6 +528,13 @@ class promise {
void set_exception(Exception&& e) noexcept {
set_exception(make_exception_ptr(std::forward<Exception>(e)));
}
+
+#if SEASTAR_COROUTINES_TS
+ void set_coroutine(future_state<T...>& state, task& coroutine) noexcept {
+ _state = &state;
+ _task = std::unique_ptr<task>(&coroutine);
+ }
+#endif
private:
template<urgent Urgent>
void do_set_value(std::tuple<T...> result) noexcept {
@@ -1223,6 +1230,15 @@ class future {
state()->ignore();
}

+#if SEASTAR_COROUTINES_TS
+ void set_coroutine(task& coroutine) noexcept {
+ assert(!state()->available());
+ assert(_promise);
+ _promise->set_coroutine(_local_state, coroutine);
+ _promise->_future = nullptr;
+ _promise = nullptr;
+ }
+#endif
private:
void set_callback(std::unique_ptr<continuation_base<T...>> callback) {
if (state()->available()) {
diff --git a/CMakeLists.txt b/CMakeLists.txt
index ade26957..4a9ff620 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -33,6 +33,10 @@ option (Seastar_ALLOC_FAILURE_INJECTION
"Enable failure injection into the Seastar allocator."
OFF)

+option (Seastar_EXPERIMENTAL_COROUTINES_TS
+ "Enable experimental support for Coroutines TS."
+ OFF)
+
#
# Add a dev build type.
#
@@ -616,6 +620,11 @@ target_compile_options (seastar
-std=${Seastar_CXX_DIALECT}
-U_FORTIFY_SOURCE)

+if (Seastar_EXPERIMENTAL_COROUTINES_TS)
+ target_compile_options (seastar PUBLIC -fcoroutines-ts -g)
+ target_compile_definitions (seastar PUBLIC SEASTAR_COROUTINES_TS)
+endif ()
+
if (Seastar_GCC6_CONCEPTS OR Concepts_FOUND)
target_compile_definitions (seastar
PUBLIC SEASTAR_HAVE_GCC6_CONCEPTS)
--
2.20.1

[Paweł Dziepak]

---
tests/unit/coroutines_test.cc | 114 ++++++++++++++++++++++++++++++++++
tests/unit/CMakeLists.txt | 5 ++
2 files changed, 119 insertions(+)
create mode 100644 tests/unit/coroutines_test.cc

diff --git a/tests/unit/coroutines_test.cc b/tests/unit/coroutines_test.cc
new file mode 100644
index 00000000..cb30947e
--- /dev/null
+++ b/tests/unit/coroutines_test.cc
@@ -0,0 +1,114 @@

+/*
+ * This file is open source software, licensed to you under the terms
+ * of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+ * distributed with this work for additional information regarding copyright
+ * ownership. You may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/*
+ * Copyright (C) 2019 ScyllaDB Ltd.
+ */
+

+#include <seastar/core/coroutine.hh>
+#include <seastar/core/future-util.hh>
+#include <seastar/testing/test_case.hh>
+
+using namespace seastar;
+
+namespace {
+
+future<int> old_fashioned_continuations() {
+ return later().then([] {
+ return 42;
+ });
+}
+
+future<int> simple_coroutine() {
+ co_await later();
+ co_return 53;
+}
+
+future<int> ready_coroutine() {
+ co_return 64;
+}
+
+future<int, double> tuple_coroutine() {
+ co_return std::tuple(1, 2.);
+}
+
+future<int> failing_coroutine() {
+ co_await later();
+ throw 42;
+}
+
+}
+
+SEASTAR_TEST_CASE(test_simple_coroutines) {
+ BOOST_REQUIRE_EQUAL(co_await old_fashioned_continuations(), 42);
+ BOOST_REQUIRE_EQUAL(co_await simple_coroutine(), 53);
+ BOOST_REQUIRE_EQUAL(ready_coroutine().get0(), 64);
+ BOOST_REQUIRE(co_await tuple_coroutine() == std::tuple(1, 2.));
+ BOOST_REQUIRE_EXCEPTION((void)co_await failing_coroutine(), int, [] (auto v) { return v == 42; });
+}
+
+SEASTAR_TEST_CASE(test_abandond_coroutine) {
+ compat::optional<future<int>> f;
+ {
+ auto p1 = promise<>();
+ auto p2 = promise<>();
+ auto p3 = promise<>();
+ f = p1.get_future().then([&] () -> future<int> {
+ p2.set_value();
+ BOOST_CHECK_THROW(co_await p3.get_future(), broken_promise);
+ co_return 1;
+ });
+ p1.set_value();
+ co_await p2.get_future();
+ }
+ BOOST_CHECK_EQUAL(co_await std::move(*f), 1);
+}
+
+SEASTAR_TEST_CASE(test_scheduling_group) {
+ auto other_sg = co_await create_scheduling_group("the other group", 10.f);
+
+ auto p1 = promise<>();
+ auto p2 = promise<>();
+
+ auto p1b = promise<>();
+ auto p2b = promise<>();
+ auto f1 = p1b.get_future();
+ auto f2 = p2b.get_future();
+
+ BOOST_REQUIRE(current_scheduling_group() == default_scheduling_group());
+ auto f_ret = with_scheduling_group(other_sg,
+ [other_sg] (future<> f1, future<> f2, promise<> p1, promise<> p2) -> future<int> {
+ BOOST_REQUIRE(current_scheduling_group() == other_sg);
+ BOOST_REQUIRE(other_sg == other_sg);
+ p1.set_value();
+ co_await std::move(f1);
+ BOOST_REQUIRE(current_scheduling_group() == other_sg);
+ p2.set_value();
+ co_await std::move(f2);
+ BOOST_REQUIRE(current_scheduling_group() == other_sg);
+ co_return 42;
+ }, p1.get_future(), p2.get_future(), std::move(p1b), std::move(p2b));
+
+ co_await std::move(f1);
+ BOOST_REQUIRE(current_scheduling_group() == default_scheduling_group());
+ p1.set_value();
+ co_await std::move(f2);
+ BOOST_REQUIRE(current_scheduling_group() == default_scheduling_group());
+ p2.set_value();
+ BOOST_REQUIRE_EQUAL(co_await std::move(f_ret), 42);
+ BOOST_REQUIRE(current_scheduling_group() == default_scheduling_group());
+}
diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt
index ac51078e..3c503899 100644
--- a/tests/unit/CMakeLists.txt
+++ b/tests/unit/CMakeLists.txt
@@ -254,6 +254,11 @@ seastar_add_test (circular_buffer_fixed_capacity
seastar_add_test (connect
SOURCES connect_test.cc)

+if (Seastar_EXPERIMENTAL_COROUTINES_TS)
+ seastar_add_test (coroutines
+ SOURCES coroutines_test.cc)
+endif ()
+
seastar_add_test (defer
SOURCES defer_test.cc)

--
2.20.1

[Paweł Dziepak]

---
demos/coroutines_demo.cc | 47 ++++++++++++++++++++++++++++++++++++++++
demos/CMakeLists.txt | 5 +++++
2 files changed, 52 insertions(+)
create mode 100644 demos/coroutines_demo.cc

diff --git a/demos/coroutines_demo.cc b/demos/coroutines_demo.cc
new file mode 100644
index 00000000..75effa70
--- /dev/null
+++ b/demos/coroutines_demo.cc
@@ -0,0 +1,47 @@

+/*
+ * This file is open source software, licensed to you under the terms
+ * of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+ * distributed with this work for additional information regarding copyright
+ * ownership. You may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/*
+ * Copyright (C) 2019 ScyllaDB Ltd.
+ */
+

+#include <seastar/core/app-template.hh>
+#include <seastar/core/coroutine.hh>
+#include <seastar/core/fstream.hh>
+#include <seastar/core/reactor.hh>
+#include <seastar/core/sleep.hh>
+
+int main(int argc, char** argv) {
+ seastar::app_template app;
+ app.run(argc, argv, [] () -> seastar::future<> {
+ std::cout << "this is a completely useless program\nplease stand by...\n";
+ auto f = seastar::parallel_for_each(std::vector<int> { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }, [] (int i) -> seastar::future<> {
+ co_await seastar::sleep(std::chrono::seconds(i));
+ std::cout << i << "\n";
+ });
+
+ auto file = co_await seastar::open_file_dma("useless_file.txt", seastar::open_flags::create | seastar::open_flags::wo);
+ auto out = seastar::make_file_output_stream(file);
+ seastar::sstring str = "nothing to see here, move along now\n";
+ co_await out.write(str);
+ co_await out.flush();
+ co_await out.close();
+
+ co_await std::move(f);
+ std::cout << "done\n";
+ });
+}
diff --git a/demos/CMakeLists.txt b/demos/CMakeLists.txt
index 202e2805..a66222dc 100644
--- a/demos/CMakeLists.txt
+++ b/demos/CMakeLists.txt
@@ -54,6 +54,11 @@ endmacro ()
seastar_add_demo (block_discard
SOURCES block_discard_demo.cc)

+if (Seastar_EXPERIMENTAL_COROUTINES_TS)
+ seastar_add_demo (coroutines
+ SOURCES coroutines_demo.cc)
+endif ()
+
seastar_add_demo (echo
SOURCES echo_demo.cc)

--
2.20.1

[Alexander Gallego]

Oh this is awesome. 

Is there any guidance w.r.t perf on either style. 

I would assume the co_await would be a bit better? 

Hopefully no longer leaks of closing a socket w/out forgetting to wrap on a .finally() block. 

Excited to try it. 

 

+
+        co_await std::move(f);
+        std::cout << "done\n";
+    });
+}
diff --git a/demos/CMakeLists.txt b/demos/CMakeLists.txt
index 202e2805..a66222dc 100644
--- a/demos/CMakeLists.txt
+++ b/demos/CMakeLists.txt
@@ -54,6 +54,11 @@ endmacro ()
 seastar_add_demo (block_discard
   SOURCES block_discard_demo.cc)

+if (Seastar_EXPERIMENTAL_COROUTINES_TS)
+  seastar_add_demo (coroutines
+    SOURCES coroutines_demo.cc)
+endif ()
+
 seastar_add_demo (echo
   SOURCES echo_demo.cc)

--
2.20.1

--
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...@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/20190124220007.21378-19-pdziepak%40scylladb.com.
For more options, visit https://groups.google.com/d/optout.

[Paweł Dziepak]

The biggest weakness of coroutines compared to seastar chains of continuations is that the latter can completely elide memory allocations for the state (if all futures involved are ready) while the former can't. This may penalise the fastest paths that do little IO. In general, coroutines are slightly more heavyweight than continuations, but they can be merged together (clang can inline them). I imagine LTO may help even more since it would allow merging coroutines across different translation units. PGO also may be of help by telling compiler which coroutines to merge (assuming PGO works with coroutines well, I haven't tried it yet).

If you want to get more insight into what code the compiler generates for coroutines you can start by playing with this minimised example: https://godbolt.org/z/ZHDtLK (this particular one is verifying that clang can inline them). Yes, there is a lot of support code generated, but hopefully it won't be a real problem.

Another issue are multiple indirect calls or branches needed to resume a coroutine. Internally, the seastar engine runs tasks. A task may represent a continuation, a seastar thread or a coroutine and is responsible of its own lifetime. So, when a future that coroutine is waiting for becomes ready the course of action is as follow:

1. A virtual function task::run_and_dispose() is called. 1st indirect call. This is however entirely seastar fault and can be avoided once coroutines stop being experimental. Right now, getting rid of it would require more intrusive changes than I feel an experimental feature justifies.

2. A task that represents a coroutine is running, all it does it to call an appropriate compiler intrinsic function (wrapped by the standard library interface) to resume a coroutine. That intrinsic is another indirect call.

3. A coroutine resume function is called. That function now needs to now jump to the right point in the corotutine (imagine it being a large switch with each case being one resume point). Depending on the number of those resume point this is going to be either a bunch of conditional branches or another indirect call.

Example: https://godbolt.org/z/sFi5ZU (see function `resume()` for point 2 and `bar(int) [clone .resume]` for point 3).

Points 2 and 3 are outside of seastar control. I am not sure yet how much of this is caused by the wording of the TS and how much is just a limitation of the current implementation.

All of this is theoretical, though. I haven't tested the performance of any real code with coroutines yet. 

 

Hopefully no longer leaks of closing a socket w/out forgetting to wrap on a .finally() block. 

Excited to try it. 

Yes, that's what we all need to do I guess. Experiment with them and learn from the experience what are the problems (and let the compiler folks if there is something they can fix).

[Avi Kivity]

On 24/01/2019 23.59, Paweł Dziepak wrote:
> gnu::used seems to be a better choice than gnu::externally_visible to
> guarantee that the function will actually exist since it is unaffected by
> 'inline' and supported by clang.

Does it perform externally_visible's function (prevent -flto from
complaining)?


Did clang complain about gnu::externally_visible? It shouldn't, really.

[Avi Kivity]

On 25/01/2019 00.00, Paweł Dziepak wrote:
> [[gnu::no_sanitize_undefined]] is not supported by clang.

So why is it complaining?!

[Avi Kivity]

On 25/01/2019 00.00, Paweł Dziepak wrote:

> Unsurprisingly, clang has issues with gcc-specific pargmas.
> ---
> include/seastar/core/temporary_buffer.hh | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/seastar/core/temporary_buffer.hh b/include/seastar/core/temporary_buffer.hh
> index 5ff4d680..48180def 100644
> --- a/include/seastar/core/temporary_buffer.hh
> +++ b/include/seastar/core/temporary_buffer.hh
> @@ -82,14 +82,18 @@ class temporary_buffer {
> temporary_buffer(const temporary_buffer&) = delete;
>
> // At least at -O1, the inline decisions are such that the following code hits https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88897.
> +#ifndef __clang__

Since the following is gcc specific, should we not check for gcc instead?

[Avi Kivity]

This needs better explanation in changelog, and wants to be outside the
series.


(I'm all for it, but it merits its own scope).

On 25/01/2019 00.00, Paweł Dziepak wrote:

None of this depends on Func and so should be a non-template function.


Why not do this in promise::abandoned()? It will cause extra work if the
future is never consumed, but that's rare. It also works if the promise
is abandoned after the schedule().


Edit: I see you also do that in abandoned(). Why is that not sufficient?


We should consider logging a warning, or perhaps capturing the current
stack trace into the broken_promise.

> ::seastar::schedule(std::make_unique<continuation<Func, T...>>(std::move(func), std::move(*state())));
> } else {
> assert(_promise);
> @@ -899,6 +921,19 @@ class future {
> }
> };
> void do_wait() noexcept {
> + if (__builtin_expect(!_promise, false)) {
> + // Encourage the compiler to move this away from the hot paths. __builtin_expect is not enough
> + // to do that. Cold lambdas work (at least for GCC8+).
> + [&] () __attribute__((cold)) {
> + try {
> + // Constructing broken_promise may throw (std::logic_error ctor is not noexcept).
> + _local_state.set_exception(std::make_exception_ptr(broken_promise{}));
> + } catch (...) {
> + _local_state.set_exception(std::current_exception());
> + }
> + }();
> + return;

Again extract, and maybe move to promise::abandoned().

[Avi Kivity]

On 24/01/2019 23.59, Paweł Dziepak wrote:

Looks good. Please split into three series:

 1. clang build fixes

 2. broken_promise

 3. coroutines

[Avi Kivity]

On 25/01/2019 00.00, Paweł Dziepak wrote:

> This patch adds all necessary glue to make coroutines work with seastar
> futures and promises. They integrate seamlessly and any function that
> returns a future may become a coroutine.
>
> seastar/core/coroutines.hh also defines some classes that should be in
> experimental/coroutine (unless that file exists, in which case it is
> included instead) so that this can work with libstdc++ which doesn't
> provide that header.

This paragraph is outdated.

Where is the "delete this" that disposes of the task? Or will the
compiler take care of that for us?

[Paweł Dziepak]

On Sun, 27 Jan 2019 at 11:12, Avi Kivity <a...@scylladb.com> wrote:

On 24/01/2019 23.59, Paweł Dziepak wrote:
> gnu::used seems to be a better choice than  gnu::externally_visible to
> guarantee that the function will actually exist since it is unaffected by
> 'inline' and supported by clang.


Does it perform externally_visible's function (prevent -flto from
complaining)?

Yes, it ensures that the symbol is exists even if LTO would think that it's not used: https://godbolt.org/z/EnFKV-

It actually does externally_visible job better than externally_visible since it works with inline functions as well. I am not sure why would one use [[gnu::externally_visible]] instead of [[gnu::used]].

 

Did clang complain about gnu::externally_visible? It shouldn't, really.

That's indeed what the standard suggests (unknown implementation-defined attributes are ignored), but I agree with clang. Some attributes change the behaviour of the program ([[gnu::packed]], [[gnu::externally_visible]]) and debugging issues caused by the compiler silently ignoring them doesn't sound like fun. FWIW GCC also warns that an unknown attribute was ignored.

[Paweł Dziepak]

On Sun, 27 Jan 2019 at 11:13, Avi Kivity <a...@scylladb.com> wrote:

On 25/01/2019 00.00, Paweł Dziepak wrote:
> [[gnu::no_sanitize_undefined]] is not supported by clang.


So why is it complaining?!

Both GCC and Clang warn about unknown attributes, which is good since some of them change the behaviour of the program and cannot be just ignored.

[Paweł Dziepak]

On Sun, 27 Jan 2019 at 11:14, Avi Kivity <a...@scylladb.com> wrote:

On 25/01/2019 00.00, Paweł Dziepak wrote:
> Unsurprisingly, clang has issues with gcc-specific pargmas.
> ---
>   include/seastar/core/temporary_buffer.hh | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/seastar/core/temporary_buffer.hh b/include/seastar/core/temporary_buffer.hh
> index 5ff4d680..48180def 100644
> --- a/include/seastar/core/temporary_buffer.hh
> +++ b/include/seastar/core/temporary_buffer.hh
> @@ -82,14 +82,18 @@ class temporary_buffer {
>       temporary_buffer(const temporary_buffer&) = delete;
>   
>       // At least at -O1, the inline decisions are such that the following code hits https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88897.
> +#ifndef __clang__


Since the following is gcc specific, should we not check for gcc instead?

Because clang lies and defines __GNUC__.

I guess the proper check should be __GNUC__ defined and __clang__ not defined, so that if we ever try to compile this with MSVC it also won't see GCC pragmas. Even better option would be to have compiler extensions sprinkled all over the code and instead have one header that takes care of all the ugly compiler-detection stuff and the rest of the code can just use appropriate definitions and macros. Seems slightly out of scope of this series though.

[Avi Kivity]

On 28/01/2019 13.05, Paweł Dziepak wrote:

On Sun, 27 Jan 2019 at 11:12, Avi Kivity <a...@scylladb.com> wrote:

On 24/01/2019 23.59, Paweł Dziepak wrote:
> gnu::used seems to be a better choice than  gnu::externally_visible to
> guarantee that the function will actually exist since it is unaffected by
> 'inline' and supported by clang.


Does it perform externally_visible's function (prevent -flto from
complaining)?

Yes, it ensures that the symbol is exists even if LTO would think that it's not used: https://godbolt.org/z/EnFKV-

It actually does externally_visible job better than externally_visible since it works with inline functions as well. I am not sure why would one use [[gnu::externally_visible]] instead of [[gnu::used]].

 

Did clang complain about gnu::externally_visible? It shouldn't, really.

That's indeed what the standard suggests (unknown implementation-defined attributes are ignored), but I agree with clang. Some attributes change the behaviour of the program ([[gnu::packed]], [[gnu::externally_visible]]) and debugging issues caused by the compiler silently ignoring them doesn't sound like fun. FWIW GCC also warns that an unknown attribute was ignored.

I can think of situations where we do want compiler-specific attributes (starting when we support multiple compiler for real), but we can defer the discussion until then. Unfortunately the standard solutions look like SEASTAR_ATTR_GNU_EXTERNALLY_VISIBLE which is quite painful to look at.

 

[Avi Kivity]

On 28/01/2019 13.11, Paweł Dziepak wrote:

On Sun, 27 Jan 2019 at 11:14, Avi Kivity <a...@scylladb.com> wrote:

On 25/01/2019 00.00, Paweł Dziepak wrote:
> Unsurprisingly, clang has issues with gcc-specific pargmas.
> ---
>   include/seastar/core/temporary_buffer.hh | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/seastar/core/temporary_buffer.hh b/include/seastar/core/temporary_buffer.hh
> index 5ff4d680..48180def 100644
> --- a/include/seastar/core/temporary_buffer.hh
> +++ b/include/seastar/core/temporary_buffer.hh
> @@ -82,14 +82,18 @@ class temporary_buffer {
>       temporary_buffer(const temporary_buffer&) = delete;
>   
>       // At least at -O1, the inline decisions are such that the following code hits https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88897.
> +#ifndef __clang__


Since the following is gcc specific, should we not check for gcc instead?

Because clang lies and defines __GNUC__.

Well, we can't complain about someone trying really hard to be compatible and missing some edge cases.

I guess the proper check should be __GNUC__ defined and __clang__ not defined, so that if we ever try to compile this with MSVC it also won't see GCC pragmas. Even better option would be to have compiler extensions sprinkled all over the code and instead have one header that takes care of all the ugly compiler-detection stuff and the rest of the code can just use appropriate definitions and macros. Seems slightly out of scope of this series though.

Yes.

[Paweł Dziepak]

I agree that definitions are the way to do it (but only if we have a CI enforcing clang, build, otherwise it is a waste of time since they will break anyway at some point), but not in this case. As I said I don't see (it is likely that I am missing something) why gnu::externally_visible exists since gnu::used is strictly better and enforcing external visibility and is supported by both compilers.

 

 

[Avi Kivity]

I was commenting on the general principle, not on this case.

[Paweł Dziepak]

promise::abandoned() could indeed call set_exception(broken_promise{}), but that would make report_failed_future() complain loudly if the future is dropped as well and that's not an erroneous case.

 

Edit: I see you also do that in abandoned(). Why is that not sufficient?

promise::abandoned() handles situations where there is a task already attached to the future. This (and do_wait() below) handle cases when there is an attempt to attach a task to a future that was already abandoned by the promise.

 

We should consider logging a warning, or perhaps capturing the current
stack trace into the broken_promise. 

>               ::seastar::schedule(std::make_unique<continuation<Func, T...>>(std::move(func), std::move(*state())));
>           } else {
>               assert(_promise);
> @@ -899,6 +921,19 @@ class future {
>           }
>       };
>       void do_wait() noexcept {
> +        if (__builtin_expect(!_promise, false)) {
> +            // Encourage the compiler to move this away from the hot paths. __builtin_expect is not enough
> +            // to do that. Cold lambdas work (at least for GCC8+).
> +            [&] () __attribute__((cold)) {
> +                try {
> +                    // Constructing broken_promise may throw (std::logic_error ctor is not noexcept).
> +                    _local_state.set_exception(std::make_exception_ptr(broken_promise{}));
> +                } catch (...) {
> +                    _local_state.set_exception(std::current_exception());
> +                }
> +            }();
> +            return;


Again extract, and maybe move to promise::abandoned().

This can be deduplicated with the same code in then(), but for the reasons I mentioned earlier moving this to promise::abandoned() is more tricky.

[Paweł Dziepak]

On Sun, 27 Jan 2019 at 11:35, Avi Kivity <a...@scylladb.com> wrote:

On 25/01/2019 00.00, Paweł Dziepak wrote:
> This patch adds all necessary glue to make coroutines work with seastar
> futures and promises. They integrate seamlessly and any function that
> returns a future may become a coroutine.
>
> seastar/core/coroutines.hh also defines some classes that should be in
> experimental/coroutine (unless that file exists, in which case it is
> included instead) so that this can work with libstdc++ which doesn't
> provide that header.


This paragraph is outdated.

Right, this is done in std-coroutine.hh now.

The compiler manages the lifetime of the coroutine promise. It is not a standalone object but a part of the coroutine state. The state gets destroyed by the compiler either implicitly at the coroutine end or explictly when std::coroutine_handle<>::destroy() is called (this never happens in this version).

[Avi Kivity]

Ok. Your patch still handles this case, but later on when the task is attached.

[Paweł Dziepak]

Thanks to the magic of Twitter I was made aware of this talk https://www.youtube.com/watch?v=wyAbV8AM9PM in which John McCall will tell you everything you need to know about coroutines in LLVM.

[Paweł Dziepak]

With futures and promises model it is possible that a promise is
destroyed before ever having a value set. In particular this may lead to
two problematic scenarios:
- promise abandons a future, and then there is an attempt to attach a
continuation or a thread to that future
- promise abandons a future to which a continuation or thread is
already attached
At the moment doing so leads to an undefined behaviour varying from the
continuation chain getting stuck to a segmentation fault. This patch
changes that so that in both of these cases a broken_promise exception
is propagated to the waiting continuation or thread thus avoiding
undefined behaviour.
---
in v1:
- extracted from Coroutines TS series
- deduplicated broken promise handling in future::do_wait and
future::schedule

include/seastar/core/future.hh | 43 +++++++++++++++++++++++++++++++++-
tests/unit/futures_test.cc | 34 +++++++++++++++++++++++++++
2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh
index e39cd41e..eee4ec35 100644

--- a/include/seastar/core/future.hh
+++ b/include/seastar/core/future.hh
@@ -103,6 +103,16 @@ void engine_exit(std::exception_ptr eptr = {});
void report_failed_future(std::exception_ptr ex);
/// \endcond

+/// \brief Exception type for broken promises
+///
+/// When a promise is broken, i.e. a promise object with an attached
+/// continuation is destroyed before setting any value or exception, an
+/// exception of `broken_promise` type is propagated to that abandoned
+/// continuation.
+struct broken_promise : std::logic_error {
+ broken_promise() : logic_error("broken promise") { }
+};
+
//
// A future/promise pair maintain one logical value (a future_state).
// To minimize allocations, the value is stored in exactly one of three

@@ -759,7 +769,10 @@ class future {

}
template <typename Func>
void schedule(Func&& func) {
- if (state()->available()) {
+ if (state()->available() || !_promise) {
+ if (__builtin_expect(!state()->available() && !_promise, false)) {

+ abandoned();
+ }

::seastar::schedule(std::make_unique<continuation<Func, T...>>(std::move(func), std::move(*state())));
} else {
assert(_promise);

@@ -801,6 +814,18 @@ class future {
}
}

+ // Used when there is to attempt to attach a continuation or a thread to a future
+ // that was abandoned by its promise.
+ [[gnu::cold]] [[gnu::noinline]]
+ void abandoned() noexcept {

+ try {
+ // Constructing broken_promise may throw (std::logic_error ctor is not noexcept).
+ _local_state.set_exception(std::make_exception_ptr(broken_promise{}));
+ } catch (...) {
+ _local_state.set_exception(std::current_exception());
+ }
+ }

+
template<typename... U>
friend class shared_future;
public:
@@ -899,6 +924,10 @@ class future {

}
};
void do_wait() noexcept {
+ if (__builtin_expect(!_promise, false)) {

+ abandoned();
+ return;

+ }
auto thread = thread_impl::get();
assert(thread);
thread_wake_task wake_task{thread, this};

@@ -1268,6 +1297,18 @@ void promise<T...>::abandoned() noexcept {

--
2.20.1

[Avi Kivity]

On 29/01/2019 22.20, Paweł Dziepak wrote:
> With futures and promises model it is possible that a promise is
> destroyed before ever having a value set. In particular this may lead to
> two problematic scenarios:
> - promise abandons a future, and then there is an attempt to attach a
> continuation or a thread to that future
> - promise abandons a future to which a continuation or thread is
> already attached
> At the moment doing so leads to an undefined behaviour varying from the
> continuation chain getting stuck to a segmentation fault. This patch
> changes that so that in both of these cases a broken_promise exception
> is propagated to the waiting continuation or thread thus avoiding
> undefined behaviour.

I like the patch, but would like to get additional review from Tomek.
This is a sensitive area.

[Nadav Har'El]

On Tue, Jan 29, 2019 at 10:20 PM Paweł Dziepak <pdzi...@scylladb.com> wrote:

With futures and promises model it is possible that a promise is
destroyed before ever having a value set.

One of the key features of the promise/future pair is that they have independent

lifetimes: One can destroy a promise before the future. This should have been true

even if the promise is destroyed before being set. So it's good you fixed it.

I think a separate question is what should happen if a promise is destroyed before

being set, meaning that the future can *never* be resolved. Is it bad that a future will

never be resolved, or in this case can never be resolved? Could somebody want

to do this deliberately (e.g., to get a future which can never resolve)? or is it much more

likely to be a bug? Do you see the new exception as a debugging aid to help a programmer

catch cases where a promise object is lost or is this a feature users would directly use?

A few more small comments below:

Nitpick: if this is what you check here, why not have a "else if(!_promise)" below (as else for if(state()->available()) instead if sticking it in the same if?

+                abandoned();
+            }
             ::seastar::schedule(std::make_unique<continuation<Func, T...>>(std::move(func), std::move(*state())));
         } else {
             assert(_promise);
@@ -801,6 +814,18 @@ class future {
         }
     }

+    // Used when there is to attempt to attach a continuation or a thread to a future
+    // that was abandoned by its promise.
+    [[gnu::cold]] [[gnu::noinline]]
+    void abandoned() noexcept {
+        try {
+            // Constructing broken_promise may throw (std::logic_error ctor is not noexcept).

What a mess :-( So should we perhaps *not* use std::logic_error and have our own, noexcept,

error hierarchy?

+            _local_state.set_exception(std::make_exception_ptr(broken_promise{}));
+        } catch (...) {

By the way, thinking of this - doesn't creating the exception and std::current_exception()

require memory allocation? What happens if *that* throws?

+            _local_state.set_exception(std::current_exception());
+        }
+    }
+
     template<typename... U>
     friend class shared_future;
 public:
@@ -899,6 +924,10 @@ class future {
         }
     };
     void do_wait() noexcept {
+        if (__builtin_expect(!_promise, false)) {

Unrelated to this patch, but the syntax of __builtin_expect is utterly unreadable and confusing (and caused us bugs in the past).

Should we perhaps consider "likely"/"unlikely" macros?

--
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...@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/20190129202023.14187-1-pdziepak%40scylladb.com.

[Avi Kivity]

On 30/01/2019 12.44, Nadav Har'El wrote:

On Tue, Jan 29, 2019 at 10:20 PM Paweł Dziepak <pdzi...@scylladb.com> wrote:

With futures and promises model it is possible that a promise is
destroyed before ever having a value set.

One of the key features of the promise/future pair is that they have independent

lifetimes: One can destroy a promise before the future. This should have been true

even if the promise is destroyed before being set. So it's good you fixed it.

I think a separate question is what should happen if a promise is destroyed before

being set, meaning that the future can *never* be resolved. Is it bad that a future will

never be resolved, or in this case can never be resolved?

It can easily happen:

  promise<> pr;

  auto f = pr.get_future();

  some_function_that_throws();

  // pass pr somewhere, call f.then();

Could somebody want

to do this deliberately (e.g., to get a future which can never resolve)?

No.

or is it much more

likely to be a bug?

It can happen due a code path not taking place due to an exceptional return.

Do you see the new exception as a debugging aid to help a programmer

catch cases where a promise object is lost or is this a feature users would directly use?

It prevents a fiber from hanging forever waiting for a future that will never come.

So that users have to handle two exception hierarchies?

+            _local_state.set_exception(std::make_exception_ptr(broken_promise{}));
+        } catch (...) {

By the way, thinking of this - doesn't creating the exception and std::current_exception()

require memory allocation? What happens if *that* throws?

You get a bad_alloc.

+            _local_state.set_exception(std::current_exception());
+        }
+    }
+
     template<typename... U>
     friend class shared_future;
 public:
@@ -899,6 +924,10 @@ class future {
         }
     };
     void do_wait() noexcept {
+        if (__builtin_expect(!_promise, false)) {

Unrelated to this patch, but the syntax of __builtin_expect is utterly unreadable and confusing (and caused us bugs in the past).

Should we perhaps consider "likely"/"unlikely" macros?

No macros.

To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/CANEVyjsX_ZGhHxsmp_G%2BBtvWrBiJ9%2B%3D1LueoqiB0p%2B8LPZq9xA%40mail.gmail.com.

[Paweł Dziepak]

On Wed, 30 Jan 2019 at 10:45, Nadav Har'El <n...@scylladb.com> wrote:

On Tue, Jan 29, 2019 at 10:20 PM Paweł Dziepak <pdzi...@scylladb.com> wrote:

With futures and promises model it is possible that a promise is
destroyed before ever having a value set.

One of the key features of the promise/future pair is that they have independent

lifetimes: One can destroy a promise before the future. This should have been true

even if the promise is destroyed before being set. So it's good you fixed it.

I think a separate question is what should happen if a promise is destroyed before

being set, meaning that the future can *never* be resolved. Is it bad that a future will

never be resolved, or in this case can never be resolved? Could somebody want

to do this deliberately (e.g., to get a future which can never resolve)? or is it much more

likely to be a bug? Do you see the new exception as a debugging aid to help a programmer

catch cases where a promise object is lost or is this a feature users would directly use?

Deliberately causing an exception to be thrown in the "expected" path is always a bug. The alternative (which I considered) to broken_promise exception would be an assertion failure, but since seastar is often used by servers, and abandoned future is not exactly a catastrophic failure (not nearly as server as memory corruption) I went for the way that gives the application a chance to recover. Again, if a broken promise is thrown this means there is a bug in the program, but one can imagine scenarios where botched up exception safety has lead to future being abandoned and an abort would be a harsh punishment.

Destroying promise without setting a value or exception is not a bug in itself. You can write code like this:

std::optional<future<>> f;

{

    auto p = promise<>();

    f.emplace(p.get_future());

}

This is perfectly valid. However, if you now attempt to attach a continuation to that future the continuation will receive a broken_promise exception.

I am not sure if I understand what you mean, but this condition is fall-through. If the sate is not available and the promise doesn't exist I call abandoned() (which sets the broken promise exception) and then proceed to call schedule().

 

+                abandoned();
+            }
             ::seastar::schedule(std::make_unique<continuation<Func, T...>>(std::move(func), std::move(*state())));
         } else {
             assert(_promise);
@@ -801,6 +814,18 @@ class future {
         }
     }

+    // Used when there is to attempt to attach a continuation or a thread to a future
+    // that was abandoned by its promise.
+    [[gnu::cold]] [[gnu::noinline]]
+    void abandoned() noexcept {
+        try {
+            // Constructing broken_promise may throw (std::logic_error ctor is not noexcept).

What a mess :-( So should we perhaps *not* use std::logic_error and have our own, noexcept,

error hierarchy?

Definitely not worth it. We should very much avoid reinventing standard library unless there is a good reason for it. Optimising exceptions is not a good reason for anything.

What we could have is to encapsulate this logic into set_exception so that it doesn't have to be repeated every time we want to set an exception in an noexcept context. Outside of the scope of this patch, though.

 

+            _local_state.set_exception(std::make_exception_ptr(broken_promise{}));
+        } catch (...) {

By the way, thinking of this - doesn't creating the exception and std::current_exception()

require memory allocation? What happens if *that* throws?

No. It is the implementation responsibility to make this noexcept. In practice there is a memory reserve for the extreme cases. Obviously, it may run out, but then the server is likely to be too far gone to ever recover.

 

+            _local_state.set_exception(std::current_exception());
+        }
+    }
+
     template<typename... U>
     friend class shared_future;
 public:
@@ -899,6 +924,10 @@ class future {
         }
     };
     void do_wait() noexcept {
+        if (__builtin_expect(!_promise, false)) {

Unrelated to this patch, but the syntax of __builtin_expect is utterly unreadable and confusing (and caused us bugs in the past).

Should we perhaps consider "likely"/"unlikely" macros?

__builtin_expect is not just about true or false. It can be used in switches as well: https://godbolt.org/z/pEXx34

However, it is true that most usages are about unlikely and likely macros (we will get [[likely]] and [[unlikely]] in C++20), so yes, we probably should add a header with some common macros.

[Tomasz Grabiec]

Looks good to me.

[Nadav Har'El]

On Wed, Jan 30, 2019 at 1:06 PM Paweł Dziepak <pdzi...@scylladb.com> wrote:

I think a separate question is what should happen if a promise is destroyed before

being set, meaning that the future can *never* be resolved. Is it bad that a future will

never be resolved, or in this case can never be resolved? Could somebody want

to do this deliberately (e.g., to get a future which can never resolve)? or is it much more

likely to be a bug? Do you see the new exception as a debugging aid to help a programmer

catch cases where a promise object is lost or is this a feature users would directly use?

Deliberately causing an exception to be thrown in the "expected" path is always a bug.

Of course. What I meant was whether somebody might want to deliberately do something like

future<> never(){

     promise p;

     return p.get_future();

}

and use it in some context that requires a future as a parameter but we never want it to be resolved.

If we wanted to support this, you'd still need to fix the code as you did, just NOT throw an exception (or assert, or anything).

But you're probably right that it's much more likely that somebody accidentally destroyed p instead of doing it deliberately as in this silly use case, so I think your idea is worthwhile as a debugging aid (and of course it's better than the current possibility of crashing).

Also, if somebody really wants to implement never() he can still do this through a promise allocated on the heap (or other mechanisms).

 

The alternative (which I considered) to broken_promise exception would be an assertion failure, but since seastar is often used by servers, and abandoned future is not exactly a catastrophic failure (not nearly as server as memory corruption) I went for the way that gives the application a chance to recover. Again, if a broken promise is thrown this means there is a bug in the program, but one can imagine scenarios where botched up exception safety has lead to future being abandoned and an abort would be a harsh punishment.

I agree.

 

 

     template <typename Func>
     void schedule(Func&& func) {
-        if (state()->available()) {
+        if (state()->available() || !_promise) {
+            if (__builtin_expect(!state()->available() && !_promise, false)) {

Nitpick: if this is what you check here, why not have a "else if(!_promise)" below (as else for if(state()->available()) instead if sticking it in the same if?

I am not sure if I understand what you mean, but this condition is fall-through.

I see... I missed that. I imagined a "return" after abandoned() ;-) Sorry.

[Paweł Dziepak]

On Wed, 30 Jan 2019 at 13:08, Nadav Har'El <n...@scylladb.com> wrote:

On Wed, Jan 30, 2019 at 1:06 PM Paweł Dziepak <pdzi...@scylladb.com> wrote:

I think a separate question is what should happen if a promise is destroyed before

being set, meaning that the future can *never* be resolved. Is it bad that a future will

never be resolved, or in this case can never be resolved? Could somebody want

to do this deliberately (e.g., to get a future which can never resolve)? or is it much more

likely to be a bug? Do you see the new exception as a debugging aid to help a programmer

catch cases where a promise object is lost or is this a feature users would directly use?

Deliberately causing an exception to be thrown in the "expected" path is always a bug.

Of course. What I meant was whether somebody might want to deliberately do something like

future<> never(){

     promise p;

     return p.get_future();

}

and use it in some context that requires a future as a parameter but we never want it to be resolved.

If we wanted to support this, you'd still need to fix the code as you did, just NOT throw an exception (or assert, or anything).

But you're probably right that it's much more likely that somebody accidentally destroyed p instead of doing it deliberately as in this silly use case, so I think your idea is worthwhile as a debugging aid (and of course it's better than the current possibility of crashing).

Also, if somebody really wants to implement never() he can still do this through a promise allocated on the heap (or other mechanisms).

How do you gracefully shut down a program that uses never() (regardless how it is implemented)?

[Paweł Dziepak]

Ping.

[Commit Bot]

From: Paweł Dziepak <pdzi...@scylladb.com>
Committer: Avi Kivity <a...@scylladb.com>
Branch: master

future: propagate broken_promise exception to abandoned continuations

With futures and promises model it is possible that a promise is
destroyed before ever having a value set. In particular this may lead to
two problematic scenarios:
- promise abandons a future, and then there is an attempt to attach a
continuation or a thread to that future
- promise abandons a future to which a continuation or thread is
already attached
At the moment doing so leads to an undefined behaviour varying from the
continuation chain getting stuck to a segmentation fault. This patch
changes that so that in both of these cases a broken_promise exception
is propagated to the waiting continuation or thread thus avoiding
undefined behaviour.

Message-Id: <20190129202023....@scylladb.com>

---
diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh

--- a/include/seastar/core/future.hh
+++ b/include/seastar/core/future.hh
@@ -103,6 +103,16 @@ void engine_exit(std::exception_ptr eptr = {});
void report_failed_future(std::exception_ptr ex);
/// \endcond

+/// \brief Exception type for broken promises
+///
+/// When a promise is broken, i.e. a promise object with an attached
+/// continuation is destroyed before setting any value or exception, an
+/// exception of `broken_promise` type is propagated to that abandoned
+/// continuation.
+struct broken_promise : std::logic_error {
+ broken_promise() : logic_error("broken promise") { }
+};
+
//
// A future/promise pair maintain one logical value (a future_state).
// To minimize allocations, the value is stored in exactly one of three

@@ -759,7 +769,10 @@ private:

}
template <typename Func>
void schedule(Func&& func) {
- if (state()->available()) {
+ if (state()->available() || !_promise) {
+ if (__builtin_expect(!state()->available() && !_promise,
false)) {
+ abandoned();
+ }
::seastar::schedule(std::make_unique<continuation<Func,
T...>>(std::move(func), std::move(*state())));
} else {
assert(_promise);

@@ -801,6 +814,18 @@ private:

}
}

+ // Used when there is to attempt to attach a continuation or a thread
to a future
+ // that was abandoned by its promise.
+ [[gnu::cold]] [[gnu::noinline]]
+ void abandoned() noexcept {
+ try {
+ // Constructing broken_promise may throw (std::logic_error
ctor is not noexcept).
+
_local_state.set_exception(std::make_exception_ptr(broken_promise{}));
+ } catch (...) {
+ _local_state.set_exception(std::current_exception());
+ }
+ }
+
template<typename... U>
friend class shared_future;
public:

@@ -899,6 +924,10 @@ private: