From d99969e0e0e9f2788d22773c9322459fb048db70 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Mon, 29 Jun 2020 18:53:16 +0300 Subject: [PATCH 1/3] api: Fix wrongly captured map of snapshots The results of get_snapshot_details() is saved in do_with, then is captured on the json callback by reference, then the do_with's future returns, so by the time callback is called the map is already free and empty. Fix by capturing the result directly on the callback. Fixes recently merged b6086526. Signed-off-by: Pavel Emelyanov --- api/storage_service.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/api/storage_service.cc b/api/storage_service.cc index 8964349c75..87276e00c9 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -983,8 +983,7 @@ void set_storage_service(http_context& ctx, routes& r) { void set_snapshot(http_context& ctx, routes& r, sharded& snap_ctl) { ss::get_snapshot_details.set(r, [&snap_ctl](std::unique_ptr req) { return snap_ctl.local().get_snapshot_details().then([] (std::unordered_map>&& result) { - return do_with(std::move(result), [](const std::unordered_map>& result) { - std::function(output_stream&&)> f = [&result](output_stream&& s) { + std::function(output_stream&&)> f = [result = std::move(result)](output_stream&& s) { return do_with(output_stream(std::move(s)), true, [&result] (output_stream& s, bool& first){ return s.write("[").then([&s, &first, &result] { return do_for_each(result, [&s, &result, &first](std::tuple>&& map){ @@ -1016,7 +1015,6 @@ void set_snapshot(http_context& ctx, routes& r, sharded& snap_ }; return make_ready_future(std::move(f)); - }); }); }); From 4f5ffa980d6035797f4fd53833b5d58f109daef3 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Mon, 29 Jun 2020 18:55:57 +0300 Subject: [PATCH 2/3] api: Fix indentation after previous patch Signed-off-by: Pavel Emelyanov --- api/storage_service.cc | 54 +++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/api/storage_service.cc b/api/storage_service.cc index 87276e00c9..6b8ab05233 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -983,38 +983,38 @@ void set_storage_service(http_context& ctx, routes& r) { void set_snapshot(http_context& ctx, routes& r, sharded& snap_ctl) { ss::get_snapshot_details.set(r, [&snap_ctl](std::unique_ptr req) { return snap_ctl.local().get_snapshot_details().then([] (std::unordered_map>&& result) { - std::function(output_stream&&)> f = [result = std::move(result)](output_stream&& s) { - return do_with(output_stream(std::move(s)), true, [&result] (output_stream& s, bool& first){ - return s.write("[").then([&s, &first, &result] { - return do_for_each(result, [&s, &result, &first](std::tuple>&& map){ - return do_with(ss::snapshots(), [&s, &first, &map](ss::snapshots& all_snapshots) { - all_snapshots.key = std::get<0>(map); - future<> f = first ? make_ready_future<>() : s.write(", "); - first = false; - std::vector snapshot; - for (auto& cf: std::get<1>(map)) { - ss::snapshot snp; - snp.ks = cf.ks; - snp.cf = cf.cf; - snp.live = cf.live; - snp.total = cf.total; - snapshot.push_back(std::move(snp)); - } - all_snapshots.value = std::move(snapshot); - return f.then([&s, &all_snapshots] { - return all_snapshots.write(s); - }); + std::function(output_stream&&)> f = [result = std::move(result)](output_stream&& s) { + return do_with(output_stream(std::move(s)), true, [&result] (output_stream& s, bool& first){ + return s.write("[").then([&s, &first, &result] { + return do_for_each(result, [&s, &result, &first](std::tuple>&& map){ + return do_with(ss::snapshots(), [&s, &first, &map](ss::snapshots& all_snapshots) { + all_snapshots.key = std::get<0>(map); + future<> f = first ? make_ready_future<>() : s.write(", "); + first = false; + std::vector snapshot; + for (auto& cf: std::get<1>(map)) { + ss::snapshot snp; + snp.ks = cf.ks; + snp.cf = cf.cf; + snp.live = cf.live; + snp.total = cf.total; + snapshot.push_back(std::move(snp)); + } + all_snapshots.value = std::move(snapshot); + return f.then([&s, &all_snapshots] { + return all_snapshots.write(s); }); }); - }).then([&s] { - return s.write("]").then([&s] { - return s.close(); - }); + }); + }).then([&s] { + return s.write("]").then([&s] { + return s.close(); }); }); - }; + }); + }; - return make_ready_future(std::move(f)); + return make_ready_future(std::move(f)); }); }); From d0d2da6ccb154100185bc02aded70b9a2bf18239 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Mon, 29 Jun 2020 18:57:35 +0300 Subject: [PATCH 3/3] api: Remove excessive capture The "result" in this lambda is already not used and can be removed Signed-off-by: Pavel Emelyanov --- api/storage_service.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/storage_service.cc b/api/storage_service.cc index 6b8ab05233..82835c9b70 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -986,7 +986,7 @@ void set_snapshot(http_context& ctx, routes& r, sharded& snap_ std::function(output_stream&&)> f = [result = std::move(result)](output_stream&& s) { return do_with(output_stream(std::move(s)), true, [&result] (output_stream& s, bool& first){ return s.write("[").then([&s, &first, &result] { - return do_for_each(result, [&s, &result, &first](std::tuple>&& map){ + return do_for_each(result, [&s, &first](std::tuple>&& map){ return do_with(ss::snapshots(), [&s, &first, &map](ss::snapshots& all_snapshots) { all_snapshots.key = std::get<0>(map); future<> f = first ? make_ready_future<>() : s.write(", ");