From defbf316bf0539f46daa4d4e07c3f39c24fc16e1 Mon Sep 17 00:00:00 2001 From: Simon Marlow Date: Mon, 10 Jul 2023 09:08:35 -0700 Subject: [PATCH 1/3] Use RocketClientChannel Differential Revision: D47326675 fbshipit-source-id: 62fa0c9c60d01e7fdde40c4825ae6604c186f198 --- cpp-channel/cpp/HeaderChannel.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/cpp-channel/cpp/HeaderChannel.cpp b/cpp-channel/cpp/HeaderChannel.cpp index 64db115e..3eb0751f 100644 --- a/cpp-channel/cpp/HeaderChannel.cpp +++ b/cpp-channel/cpp/HeaderChannel.cpp @@ -1,7 +1,7 @@ // Copyright (c) Facebook, Inc. and its affiliates. #include -#include +#include using namespace apache::thrift; using namespace folly; @@ -15,7 +15,7 @@ typedef folly::AsyncTransport::UniquePtr (*MakeTransport)( folly::EventBase* eb, size_t conn_timeout); -HeaderClientChannel::Ptr* newHeaderChannel( +RocketClientChannel::Ptr* newHeaderChannel( const char* host_str, size_t host_len, size_t port, @@ -30,18 +30,16 @@ HeaderClientChannel::Ptr* newHeaderChannel( // Construction of the socket needs to be in the event base thread auto f = folly::via(eb, [=, &addr] { auto transport = makeTransport(addr, eb, conn_timeout); - auto chan = HeaderClientChannel::newChannel( - std::move(transport), - HeaderClientChannel::Options().setProtocolId(protocol_id)); - + auto chan = RocketClientChannel::newChannel(std::move(transport)); + chan->setProtocolId(protocol_id); chan->setTimeout(send_timeout + recv_timeout); chan->getTransport()->setSendTimeout(send_timeout); return chan; }); - return new HeaderClientChannel::Ptr(std::move(f).get()); + return new RocketClientChannel::Ptr(std::move(f).get()); } -void deleteHeaderChannel(HeaderClientChannel::Ptr* ch) noexcept { +void deleteHeaderChannel(RocketClientChannel::Ptr* ch) noexcept { auto h = std::move(*ch); delete ch; // Destruction needs to be done in the event base thread too From 05bb2d0f3c4a0704bcc897da258995ffd85a9197 Mon Sep 17 00:00:00 2001 From: Simon Marlow Date: Mon, 10 Jul 2023 09:08:35 -0700 Subject: [PATCH 2/3] Fix linking failure Summary: The test is still broken, but this gets it further. Differential Revision: D47327063 fbshipit-source-id: 2ab10975778fd5ab4101c18ac7c50e795d80fcf1 --- cpp-channel/thrift-cpp-channel.cabal | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp-channel/thrift-cpp-channel.cabal b/cpp-channel/thrift-cpp-channel.cabal index 3a31e0a7..6ec14e7e 100644 --- a/cpp-channel/thrift-cpp-channel.cabal +++ b/cpp-channel/thrift-cpp-channel.cabal @@ -144,6 +144,7 @@ test-suite header-channel test/if/gen-cpp2/Calculator.cpp test/if/gen-cpp2/AdderAsyncClient.cpp test/if/gen-cpp2/Adder.cpp + test/if/gen-cpp2/math_data.cpp test/if/gen-cpp2/math_types.cpp test/if/gen-cpp2/math_metadata.cpp extra-libraries: From 8ed0e9393e52dc899e82df4f7bcd3ee0317237a0 Mon Sep 17 00:00:00 2001 From: Simon Marlow Date: Mon, 10 Jul 2023 09:08:54 -0700 Subject: [PATCH 3/3] Disable broken test (#113) Summary: Pull Request resolved: https://github.com/facebookincubator/hsthrift/pull/113 See comment Reviewed By: phlalx Differential Revision: D47334480 fbshipit-source-id: fbc68cca56bc1e4ae4249d9d1851623008a0761e --- .github/workflows/ci-getdeps.yml | 2 +- cpp-channel/thrift-cpp-channel.cabal | 18 +++++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci-getdeps.yml b/.github/workflows/ci-getdeps.yml index e18a4374..5bf5c795 100644 --- a/.github/workflows/ci-getdeps.yml +++ b/.github/workflows/ci-getdeps.yml @@ -60,5 +60,5 @@ jobs: run: cabal build all working-directory: ./_sdists - name: Run testsuites - run: cabal test mangle fb-util thrift-compiler thrift-lib thrift-cpp-channel thrift-server thrift-tests --keep-going + run: cabal test mangle fb-util thrift-compiler thrift-lib thrift-server thrift-tests --keep-going working-directory: ./_sdists diff --git a/cpp-channel/thrift-cpp-channel.cabal b/cpp-channel/thrift-cpp-channel.cabal index 6ec14e7e..ea6297d6 100644 --- a/cpp-channel/thrift-cpp-channel.cabal +++ b/cpp-channel/thrift-cpp-channel.cabal @@ -132,6 +132,11 @@ flag tests_use_ipv4 default: False manual: True +flag include_broken_tests + description: Include broken tests + default: False + manual: True + test-suite header-channel import: fb-haskell, fb-cpp type: exitcode-stdio-1.0 @@ -144,8 +149,8 @@ test-suite header-channel test/if/gen-cpp2/Calculator.cpp test/if/gen-cpp2/AdderAsyncClient.cpp test/if/gen-cpp2/Adder.cpp - test/if/gen-cpp2/math_data.cpp test/if/gen-cpp2/math_types.cpp + test/if/gen-cpp2/math_data.cpp test/if/gen-cpp2/math_metadata.cpp extra-libraries: thriftmetadata @@ -177,3 +182,14 @@ test-suite header-channel unordered-containers, deepseq, STMonadTrans + + -- This test is broken when built from github. For some reason the + -- requests timeout. It seems to be a subtle timing issue of some + -- kind inside fbthrift, because I tried tracing through the code + -- using gdb and when using breakpoints the test will sometimes + -- pass. Without a deeper understanding of the internals of fbthrift + -- I can't get any further, so just ignoring the test for now. It + -- passes on the internal CI so I'm pretty sure it's not an issue + -- with the test itself. + if !flag(include_broken_tests) + buildable: False