From 24eff3ab6d52a3ea96314ab758fe60e08605e8cf Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Wed, 1 May 2019 23:32:16 -0500 Subject: [PATCH 01/10] Add API support for optional arguments FC_API now supports optional arguments! Any trailing arguments of an fc::optional type are now optional, and do not need to be supplied. If omitted, they will be inferred to be null optionals. --- include/fc/api.hpp | 70 ++++++++++++++++++++++++++++--- include/fc/rpc/api_connection.hpp | 20 ++++++--- 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/include/fc/api.hpp b/include/fc/api.hpp index 3f68750..595b5dd 100644 --- a/include/fc/api.hpp +++ b/include/fc/api.hpp @@ -3,6 +3,10 @@ #include #include #include +#include +#include +#include +#include // ms visual c++ (as of 2013) doesn't accept the standard syntax for calling a // templated member function (foo->template bar();) @@ -13,13 +17,67 @@ #endif namespace fc { - struct identity_member { + namespace detail { + namespace hana = boost::hana; + + /// This metafunction determines whether its template argument is an instantiation of fc::optional + template struct is_optional : public std::false_type {}; + template struct is_optional> : public std::true_type {}; + /// This metafunction determines whether all of its template arguments are instantiations of fc::optional + template struct all_optionals; + template<> struct all_optionals<> : public std::true_type {}; + template struct all_optionals : public std::false_type {}; + template struct all_optionals, Ts...> : public all_optionals {}; + + /// A wrapper of std::function allowing callers to omit the last several arguments if those arguments are + /// fc::optional types. i.e. given a function taking (int, double, bool, fc::optional, fc::optional), + /// whereas normally the last two arguments must be provided, this template allows them to be omitted. + /// Note that this only applies to trailing optional arguments, i.e. given a callable taking + /// (fc::optional, int, fc::optional), only the last argument can be omitted. + template + struct optionals_callable : public std::function { + using std::function::operator(); + + /// Overload the function call operator, enabled if the caller provides fewer arguments than there are parameters. + /// Pads out the provided arguments with default-constructed optionals, checking that they are indeed optional types + template + std::enable_if_t operator()(Args... args) { + auto arguments = hana::make_tuple(std::forward(args)...); + // Get the parameter types corresponding to the omitted arguments + auto optional_types = hana::take_back(hana::tuple_t, + hana::size_c); + // Transform the types into default-constructed values, checking that they are optional types + auto optional_values = hana::transform(optional_types, [](auto hanatype) { + using type = std::decay_t; + static_assert(is_optional::value, + "All omitted arguments must correspond to optional parameters."); + return type(); + }); + auto padded_arguments = hana::concat(arguments, optional_values); + return hana::unpack(padded_arguments, + [this](auto... params) { return (*this)(std::forward(params)...); }); + } + }; + } + + // This is no longer used and probably no longer can be used without generalizing the infrastructure around it, but I + // kept it because it is informative. +// struct identity_member { +// template +// static std::function functor( P&& p, R (C::*mem_func)(Args...) ); +// template +// static std::function functor( P&& p, R (C::*mem_func)(Args...)const ); +// }; + /// Used as the Transform template parameter for APIs, this type has two main purposes: first, it reads the argument + /// list and return type of a method into template parameters; and second, it uses those types in conjunction with the + /// optionals_callable template above to create a function pointer which supports optional arguments. + struct identity_member_with_optionals { template - static std::function functor( P&& p, R (C::*mem_func)(Args...) ); + static detail::optionals_callable functor( P&& p, R (C::*mem_func)(Args...) ); template - static std::function functor( P&& p, R (C::*mem_func)(Args...)const ); + static detail::optionals_callable functor( P&& p, R (C::*mem_func)(Args...)const ); }; - + template< typename Interface, typename Transform > struct vtable : public std::enable_shared_from_this> { private: vtable(); }; @@ -57,13 +115,13 @@ namespace fc { // defined in api_connection.hpp template< typename T > - api as(); + api as(); }; typedef std::shared_ptr< api_base > api_ptr; class api_connection; - template + template class api : public api_base { public: typedef vtable vtable_type; diff --git a/include/fc/rpc/api_connection.hpp b/include/fc/rpc/api_connection.hpp index 055f6dd..8e1df24 100644 --- a/include/fc/rpc/api_connection.hpp +++ b/include/fc/rpc/api_connection.hpp @@ -33,7 +33,9 @@ namespace fc { template std::function bind_first_arg( const std::function& f, Arg0 a0 ) { - return [=]( Args... args ) { return f( a0, args... ); }; + // Capture a0 this way because of a {compiler,fc,???} bug that causes optional() to be incorrectly + // captured as optional(false). + return [f, a0 = std::decay_t(a0)]( Args... args ) { return f( a0, args... ); }; } template R call_generic( const std::function& f, variants::const_iterator a0, variants::const_iterator e, uint32_t max_depth = 1 ) @@ -44,9 +46,11 @@ namespace fc { template R call_generic( const std::function& f, variants::const_iterator a0, variants::const_iterator e, uint32_t max_depth ) { - FC_ASSERT( a0 != e ); + bool optional_args = all_optionals, std::decay_t...>::value; + FC_ASSERT( a0 != e || optional_args ); FC_ASSERT( max_depth > 0, "Recursion depth exceeded!" ); - return call_generic( bind_first_arg( f, a0->as< typename std::decay::type >( max_depth - 1 ) ), a0+1, e, max_depth - 1 ); + auto arg = (a0 == e)? std::decay_t() : a0->as>(max_depth - 1); + return call_generic( bind_first_arg( f, arg ), a0+1, e, max_depth - 1 ); } template @@ -137,7 +141,9 @@ namespace fc { template std::function bind_first_arg( const std::function& f, Arg0 a0 )const { - return [=]( Args... args ) { return f( a0, args... ); }; + // Capture a0 this way because of a {compiler,fc,???} bug that causes optional() to be incorrectly + // captured as optional(false). + return [f, a0 = std::decay_t(a0)]( Args... args ) { return f( a0, args... ); }; } template @@ -172,9 +178,11 @@ namespace fc { template R call_generic( const std::function& f, variants::const_iterator a0, variants::const_iterator e, uint32_t max_depth ) { - FC_ASSERT( a0 != e, "too few arguments passed to method" ); + bool optional_args = detail::all_optionals, std::decay_t...>::value; + FC_ASSERT( a0 != e || optional_args, "too few arguments passed to method" ); FC_ASSERT( max_depth > 0, "Recursion depth exceeded!" ); - return call_generic( this->bind_first_arg( f, a0->as< typename std::decay::type >( max_depth - 1 ) ), a0+1, e, max_depth - 1 ); + auto arg = (a0 == e)? std::decay_t() : a0->as>(max_depth - 1); + return call_generic( this->bind_first_arg( f, arg ), a0+1, e, max_depth - 1 ); } struct api_visitor From eb48480246d9bf98462205e3fca2ca811ae8c10f Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Fri, 3 May 2019 20:40:33 -0500 Subject: [PATCH 02/10] Do optional API args without hana So sad... the hana version was way more expressive... But hey, I'm kinda proud I actually got this working! :D --- include/fc/api.hpp | 57 ++++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 20 deletions(-) diff --git a/include/fc/api.hpp b/include/fc/api.hpp index 595b5dd..c9fb41b 100644 --- a/include/fc/api.hpp +++ b/include/fc/api.hpp @@ -3,10 +3,6 @@ #include #include #include -#include -#include -#include -#include // ms visual c++ (as of 2013) doesn't accept the standard syntax for calling a // templated member function (foo->template bar();) @@ -18,8 +14,6 @@ namespace fc { namespace detail { - namespace hana = boost::hana; - /// This metafunction determines whether its template argument is an instantiation of fc::optional template struct is_optional : public std::false_type {}; template struct is_optional> : public std::true_type {}; @@ -38,24 +32,47 @@ namespace fc { struct optionals_callable : public std::function { using std::function::operator(); + template + struct short_pack {}; + /// This metafunction removes the first several types from a variadic parameter pack of types. + /// The first parameter is the number of arguments to remove from the beginning of the pack. + /// All subsequent parameters are types in the list to be cut + /// The result pack_cutter<...>::type is a short_pack + template + struct pack_cutter; + template + struct pack_cutter_impl; + template + struct pack_cutter_impl<0, void, Types...> { + static_assert(all_optionals::value, "All omitted arguments must correspond to optional parameters."); + using type = short_pack; + }; + template + struct pack_cutter_impl, T, Types...> + : public pack_cutter_impl {}; + template + struct pack_cutter : public pack_cutter_impl {}; + template + using pack_cutter_t = typename pack_cutter::type; + + template + R call_function(F&& f, short_pack) { + return f(OptionalTypes()...); + } + /// Overload the function call operator, enabled if the caller provides fewer arguments than there are parameters. /// Pads out the provided arguments with default-constructed optionals, checking that they are indeed optional types template std::enable_if_t operator()(Args... args) { - auto arguments = hana::make_tuple(std::forward(args)...); - // Get the parameter types corresponding to the omitted arguments - auto optional_types = hana::take_back(hana::tuple_t, - hana::size_c); - // Transform the types into default-constructed values, checking that they are optional types - auto optional_values = hana::transform(optional_types, [](auto hanatype) { - using type = std::decay_t; - static_assert(is_optional::value, - "All omitted arguments must correspond to optional parameters."); - return type(); - }); - auto padded_arguments = hana::concat(arguments, optional_values); - return hana::unpack(padded_arguments, - [this](auto... params) { return (*this)(std::forward(params)...); }); + // Partially apply with the arguments provided + auto partial_function = [this, &args...](auto&&... rest) { + return (*this)(std::forward(args)..., std::move(rest)...); + }; + // Cut the provided arguments' types out of the Parameters list, and store the rest in a dummy type + pack_cutter_t...> dummy; + // Pass the partially applied function and the dummy type to another function which can deduce the optional + // types and call the function with default instantiations of those types + return call_function(std::move(partial_function), dummy); } }; } From a89e30187fb2651e7e096db0626b2d83eae3ff1f Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Fri, 3 May 2019 21:51:15 -0500 Subject: [PATCH 03/10] API Tests General cleanup of a lot of nonsense Add tests of optional parameters --- .travis.yml | 2 +- include/fc/api.hpp | 2 +- tests/api.cpp | 169 ++++++++++++++++----------------------------- 3 files changed, 60 insertions(+), 113 deletions(-) diff --git a/.travis.yml b/.travis.yml index 63cf756..6b1d981 100644 --- a/.travis.yml +++ b/.travis.yml @@ -30,7 +30,7 @@ script: - 'which build-wrapper-linux-x86-64 && build-wrapper-linux-x86-64 --out-dir bw-output make -j 2 || make -j 2' - set -o pipefail - tests/run-parallel-tests.sh tests/all_tests - - "tests/api 2>&1 | grep -vE 'callback result 9|remote_calc->add. 4, 5 .: 9|set callback|] \\.$'" + - tests/api 2>&1 | cat - tests/hmac_test 2>&1 | cat - tests/ecc_test README.md 2>&1 | cat - 'find CMakeFiles/fc.dir -type d | while read d; do gcov -o "$d" "${d/CMakeFiles*.dir/./}"/*.cpp; done >/dev/null' diff --git a/include/fc/api.hpp b/include/fc/api.hpp index c9fb41b..d75a159 100644 --- a/include/fc/api.hpp +++ b/include/fc/api.hpp @@ -48,7 +48,7 @@ namespace fc { using type = short_pack; }; template - struct pack_cutter_impl, T, Types...> + struct pack_cutter_impl, T, Types...> : public pack_cutter_impl {}; template struct pack_cutter : public pack_cutter_impl {}; diff --git a/tests/api.cpp b/tests/api.cpp index fbe42f2..5734758 100644 --- a/tests/api.cpp +++ b/tests/api.cpp @@ -28,6 +28,17 @@ class login_api }; FC_API( login_api, (get_calc)(test) ); + +class optionals_api +{ +public: + std::string foo( const std::string& first, const fc::optional& second, + const fc::optional& third ) { + return fc::json::to_string(fc::variants{first, {second, 2}, {third, 2}}); + } +}; +FC_API( optionals_api, (foo) ); + using namespace fc; class some_calculator @@ -59,8 +70,8 @@ int main( int argc, char** argv ) try { fc::api calc_api( std::make_shared() ); - fc::http::websocket_server server; - server.on_connection([&]( const websocket_connection_ptr& c ){ + auto server = std::make_shared(); + server->on_connection([&]( const websocket_connection_ptr& c ){ auto wsc = std::make_shared(c, MAX_DEPTH); auto login = std::make_shared(); login->calc = calc_api; @@ -68,124 +79,60 @@ int main( int argc, char** argv ) c->set_session_data( wsc ); }); - server.listen( 8090 ); - server.start_accept(); + server->listen( 8090 ); + server->start_accept(); - for( uint32_t i = 0; i < 5000; ++i ) - { - try { - fc::http::websocket_client client; - auto con = client.connect( "ws://localhost:8090" ); - auto apic = std::make_shared(con, MAX_DEPTH); - auto remote_login_api = apic->get_remote_api(); - auto remote_calc = remote_login_api->get_calc(); - remote_calc->on_result( []( uint32_t r ) { elog( "callback result ${r}", ("r",r) ); } ); - wdump((remote_calc->add( 4, 5 ))); - } catch ( const fc::exception& e ) - { - edump((e.to_detail_string())); - } - } - wlog( "exit scope" ); - } - catch( const fc::exception& e ) - { - edump((e.to_detail_string())); - } - wlog( "returning now..." ); - - return 0; + try { + auto client = std::make_shared(); + auto con = client->connect( "ws://localhost:8090" ); + auto apic = std::make_shared(con, MAX_DEPTH); + auto remote_login_api = apic->get_remote_api(); + auto remote_calc = remote_login_api->get_calc(); + bool remote_triggered = false; + remote_calc->on_result( [&remote_triggered]( uint32_t r ) { remote_triggered = true; } ); + FC_ASSERT(remote_calc->add( 4, 5 ) == 9); + FC_ASSERT(remote_triggered); - some_calculator calc; - variant_calculator vcalc; + client.reset(); + fc::usleep(fc::milliseconds(100)); + server.reset(); + } FC_LOG_AND_RETHROW() + } FC_LOG_AND_RETHROW() - fc::api api_calc( &calc ); - fc::api api_vcalc( &vcalc ); - fc::api api_nested_calc( api_calc ); - - wdump( (api_calc->add(5,4)) ); - wdump( (api_calc->sub(5,4)) ); - wdump( (api_vcalc->add(5,4)) ); - wdump( (api_vcalc->sub(5,4)) ); - wdump( (api_nested_calc->sub(5,4)) ); - wdump( (api_nested_calc->sub(5,4)) ); - - /* - variants v = { 4, 5 }; - auto g = to_generic( api_calc->add ); - auto r = call_generic( api_calc->add, v.begin(), v.end() ); - wdump((r)); - wdump( (g(v)) ); - */ - - /* try { - fc::api_server server; - auto api_id = server.register_api( api_calc ); - wdump( (api_id) ); - auto result = server.call( api_id, "add", {4, 5} ); - wdump( (result) ); - } catch ( const fc::exception& e ) - { - elog( "${e}", ("e",e.to_detail_string() ) ); - } + auto optionals = std::make_shared(); + fc::api oapi(optionals); + FC_ASSERT(oapi->foo("a") == "[\"a\",null,null]"); + FC_ASSERT(oapi->foo("a", "b") == "[\"a\",\"b\",null]"); + FC_ASSERT(oapi->foo("a", "b", "c") == "[\"a\",\"b\",\"c\"]"); + FC_ASSERT(oapi->foo("a", {}, "c") == "[\"a\",null,\"c\"]"); - ilog( "------------------ NESTED TEST --------------" ); - try { - login_api napi_impl; - napi_impl.calc = api_calc; - fc::api napi(&napi_impl); + auto server = std::make_shared(); + server->on_connection([&]( const websocket_connection_ptr& c ){ + auto wsc = std::make_shared(*c, MAX_DEPTH); + wsc->register_api(fc::api(optionals)); + c->set_session_data( wsc ); + }); - fc::api_server server; - auto api_id = server.register_api( napi ); - wdump( (api_id) ); - auto result = server.call( api_id, "get_calc" ); - wdump( (result) ); - result = server.call( result.as_uint64(), "add", {4,5} ); - wdump( (result) ); + server->listen( 8090 ); + server->start_accept(); + try { + auto client = std::make_shared(); + auto con = client->connect( "ws://localhost:8090" ); + auto apic = std::make_shared(*con, MAX_DEPTH); + auto remote_optionals = apic->get_remote_api(); - fc::api serv( &server ); + FC_ASSERT(remote_optionals->foo("a") == "[\"a\",null,null]"); + FC_ASSERT(remote_optionals->foo("a", "b") == "[\"a\",\"b\",null]"); + FC_ASSERT(remote_optionals->foo("a", "b", "c") == "[\"a\",\"b\",\"c\"]"); + FC_ASSERT(remote_optionals->foo("a", {}, "c") == "[\"a\",null,\"c\"]"); - fc::api_client apic( serv ); - - fc::api remote_api = apic; - - - auto remote_calc = remote_api->get_calc(); - int r = remote_calc->add( 4, 5 ); - idump( (r) ); - - } catch ( const fc::exception& e ) - { - elog( "${e}", ("e",e.to_detail_string() ) ); - } - */ - - ilog( "------------------ NESTED TEST --------------" ); - try { - login_api napi_impl; - napi_impl.calc = api_calc; - fc::api napi(&napi_impl); - - - auto client_side = std::make_shared(MAX_DEPTH); - auto server_side = std::make_shared(MAX_DEPTH); - server_side->set_remote_connection( client_side ); - client_side->set_remote_connection( server_side ); - - server_side->register_api( napi ); - - fc::api remote_api = client_side->get_remote_api(); - - auto remote_calc = remote_api->get_calc(); - int r = remote_calc->add( 4, 5 ); - idump( (r) ); - - } catch ( const fc::exception& e ) - { - elog( "${e}", ("e",e.to_detail_string() ) ); - } + client.reset(); + fc::usleep(fc::milliseconds(100)); + server.reset(); + } FC_LOG_AND_RETHROW() + } FC_LOG_AND_RETHROW() return 0; } From 94a18cfccc33ee15b8b1b88354ccf635edabb17c Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Sat, 4 May 2019 16:57:55 -0500 Subject: [PATCH 04/10] Roll API tests into all_tests --- .travis.yml | 1 - tests/CMakeLists.txt | 5 +--- tests/{api.cpp => api_tests.cpp} | 42 +++++++++++++++----------------- 3 files changed, 20 insertions(+), 28 deletions(-) rename tests/{api.cpp => api_tests.cpp} (77%) diff --git a/.travis.yml b/.travis.yml index 6b1d981..6aca7d5 100644 --- a/.travis.yml +++ b/.travis.yml @@ -30,7 +30,6 @@ script: - 'which build-wrapper-linux-x86-64 && build-wrapper-linux-x86-64 --out-dir bw-output make -j 2 || make -j 2' - set -o pipefail - tests/run-parallel-tests.sh tests/all_tests - - tests/api 2>&1 | cat - tests/hmac_test 2>&1 | cat - tests/ecc_test README.md 2>&1 | cat - 'find CMakeFiles/fc.dir -type d | while read d; do gcov -o "$d" "${d/CMakeFiles*.dir/./}"/*.cpp; done >/dev/null' diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e968bb3..9e395c3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,7 +1,3 @@ - -add_executable( api api.cpp ) -target_link_libraries( api fc ) - if( ECC_IMPL STREQUAL secp256k1 ) add_executable( blind all_tests.cpp crypto/blind.cpp ) target_link_libraries( blind fc ) @@ -55,5 +51,6 @@ add_executable( all_tests all_tests.cpp utf8_test.cpp variant_test.cpp logging_tests.cpp + api_tests.cpp ) target_link_libraries( all_tests fc ) diff --git a/tests/api.cpp b/tests/api_tests.cpp similarity index 77% rename from tests/api.cpp rename to tests/api_tests.cpp index 5734758..b334691 100644 --- a/tests/api.cpp +++ b/tests/api_tests.cpp @@ -1,3 +1,5 @@ +#include + #include #include #include @@ -50,23 +52,15 @@ class some_calculator void on_result2( const std::function& cb, int test ){} std::function _cb; }; -class variant_calculator -{ - public: - double add( fc::variant a, fc::variant b ) { return a.as_double()+b.as_double(); } - double sub( fc::variant a, fc::variant b ) { return a.as_double()-b.as_double(); } - void on_result( const std::function& cb ) { wlog("set callback"); _cb = cb; return ; } - void on_result2( const std::function& cb, int test ){} - std::function _cb; -}; using namespace fc::http; using namespace fc::rpc; #define MAX_DEPTH 10 -int main( int argc, char** argv ) -{ +BOOST_AUTO_TEST_SUITE(api_tests) + +BOOST_AUTO_TEST_CASE(login_test) { try { fc::api calc_api( std::make_shared() ); @@ -90,22 +84,24 @@ int main( int argc, char** argv ) auto remote_calc = remote_login_api->get_calc(); bool remote_triggered = false; remote_calc->on_result( [&remote_triggered]( uint32_t r ) { remote_triggered = true; } ); - FC_ASSERT(remote_calc->add( 4, 5 ) == 9); - FC_ASSERT(remote_triggered); + BOOST_CHECK_EQUAL(remote_calc->add( 4, 5 ), 9); + BOOST_CHECK(remote_triggered); client.reset(); fc::usleep(fc::milliseconds(100)); server.reset(); } FC_LOG_AND_RETHROW() } FC_LOG_AND_RETHROW() +} +BOOST_AUTO_TEST_CASE(optionals_test) { try { auto optionals = std::make_shared(); fc::api oapi(optionals); - FC_ASSERT(oapi->foo("a") == "[\"a\",null,null]"); - FC_ASSERT(oapi->foo("a", "b") == "[\"a\",\"b\",null]"); - FC_ASSERT(oapi->foo("a", "b", "c") == "[\"a\",\"b\",\"c\"]"); - FC_ASSERT(oapi->foo("a", {}, "c") == "[\"a\",null,\"c\"]"); + BOOST_CHECK_EQUAL(oapi->foo("a"), "[\"a\",null,null]"); + BOOST_CHECK_EQUAL(oapi->foo("a", "b"), "[\"a\",\"b\",null]"); + BOOST_CHECK_EQUAL(oapi->foo("a", "b", "c"), "[\"a\",\"b\",\"c\"]"); + BOOST_CHECK_EQUAL(oapi->foo("a", {}, "c"), "[\"a\",null,\"c\"]"); auto server = std::make_shared(); server->on_connection([&]( const websocket_connection_ptr& c ){ @@ -123,16 +119,16 @@ int main( int argc, char** argv ) auto apic = std::make_shared(*con, MAX_DEPTH); auto remote_optionals = apic->get_remote_api(); - FC_ASSERT(remote_optionals->foo("a") == "[\"a\",null,null]"); - FC_ASSERT(remote_optionals->foo("a", "b") == "[\"a\",\"b\",null]"); - FC_ASSERT(remote_optionals->foo("a", "b", "c") == "[\"a\",\"b\",\"c\"]"); - FC_ASSERT(remote_optionals->foo("a", {}, "c") == "[\"a\",null,\"c\"]"); + BOOST_CHECK_EQUAL(remote_optionals->foo("a"), "[\"a\",null,null]"); + BOOST_CHECK_EQUAL(remote_optionals->foo("a", "b"), "[\"a\",\"b\",null]"); + BOOST_CHECK_EQUAL(remote_optionals->foo("a", "b", "c"), "[\"a\",\"b\",\"c\"]"); + BOOST_CHECK_EQUAL(remote_optionals->foo("a", {}, "c"), "[\"a\",null,\"c\"]"); client.reset(); fc::usleep(fc::milliseconds(100)); server.reset(); } FC_LOG_AND_RETHROW() } FC_LOG_AND_RETHROW() - - return 0; } + +BOOST_AUTO_TEST_SUITE_END() From bb01f3e925aef4bd6d2df4e339352e99734b5a6e Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Sat, 4 May 2019 17:44:33 -0500 Subject: [PATCH 05/10] Use random port for API tests Add missing functionality to websocket_client and websocket_server to make API tests more reliable and to make it possible to use a random port for the tests. --- include/fc/network/http/websocket.hpp | 8 +++++ src/network/http/websocket.cpp | 46 +++++++++++++++++++++++++-- tests/api_tests.cpp | 20 ++++++++---- 3 files changed, 65 insertions(+), 9 deletions(-) diff --git a/include/fc/network/http/websocket.hpp b/include/fc/network/http/websocket.hpp index 8837057..8225823 100644 --- a/include/fc/network/http/websocket.hpp +++ b/include/fc/network/http/websocket.hpp @@ -50,8 +50,13 @@ namespace fc { namespace http { void on_connection( const on_connection_handler& handler); void listen( uint16_t port ); void listen( const fc::ip::endpoint& ep ); + uint16_t get_listening_port(); void start_accept(); + void stop_listening(); + void close(); + void synchronous_close(); + private: friend class detail::websocket_server_impl; std::unique_ptr my; @@ -83,6 +88,9 @@ namespace fc { namespace http { websocket_connection_ptr connect( const std::string& uri ); websocket_connection_ptr secure_connect( const std::string& uri ); + + void close(); + void synchronous_close(); private: std::unique_ptr my; std::unique_ptr smy; diff --git a/src/network/http/websocket.cpp b/src/network/http/websocket.cpp index 81aa873..75b882b 100644 --- a/src/network/http/websocket.cpp +++ b/src/network/http/websocket.cpp @@ -431,6 +431,7 @@ namespace fc { namespace http { typedef websocket_client_type::connection_ptr websocket_client_connection_type; typedef websocket_tls_client_type::connection_ptr websocket_tls_client_connection_type; + using websocketpp::connection_hdl; class websocket_client_impl { @@ -484,6 +485,7 @@ namespace fc { namespace http { websocket_client_type _client; websocket_connection_ptr _connection; std::string _uri; + fc::optional _hdl; }; @@ -625,11 +627,35 @@ namespace fc { namespace http { } void websocket_server::listen( const fc::ip::endpoint& ep ) { - my->_server.listen( boost::asio::ip::tcp::endpoint( boost::asio::ip::address_v4(uint32_t(ep.get_address())),ep.port()) ); + my->_server.listen( boost::asio::ip::tcp::endpoint( boost::asio::ip::address_v4(uint32_t(ep.get_address())),ep.port()) ); + } + + uint16_t websocket_server::get_listening_port() + { + websocketpp::lib::asio::error_code ec; + return my->_server.get_local_endpoint(ec).port(); } void websocket_server::start_accept() { - my->_server.start_accept(); + my->_server.start_accept(); + } + + void websocket_server::stop_listening() + { + my->_server.stop_listening(); + } + + void websocket_server::close() + { + for (auto& connection : my->_connections) + my->_server.close(connection.first, websocketpp::close::status::normal, "Goodbye"); + } + + void websocket_server::synchronous_close() + { + close(); + while (!my->_connections.empty()) + fc::yield(); } @@ -678,6 +704,7 @@ namespace fc { namespace http { my->_connected = fc::promise::ptr( new fc::promise("websocket::connect") ); my->_client.set_open_handler( [=]( websocketpp::connection_hdl hdl ){ + my->_hdl = hdl; auto con = my->_client.get_con_from_hdl(hdl); my->_connection = std::make_shared>( con ); my->_closed = fc::promise::ptr( new fc::promise("websocket::closed") ); @@ -717,7 +744,20 @@ namespace fc { namespace http { smy->_client.connect(con); smy->_connected->wait(); return smy->_connection; - } FC_CAPTURE_AND_RETHROW( (uri) ) } + } FC_CAPTURE_AND_RETHROW( (uri) ) } + + void websocket_client::close() + { + if (my->_hdl) + my->_client.close(*my->_hdl, websocketpp::close::status::normal, "Goodbye"); + } + + void websocket_client::synchronous_close() + { + close(); + if (my->_closed) + my->_closed->wait(); + } websocket_connection_ptr websocket_tls_client::connect( const std::string& uri ) { try { diff --git a/tests/api_tests.cpp b/tests/api_tests.cpp index b334691..00997ad 100644 --- a/tests/api_tests.cpp +++ b/tests/api_tests.cpp @@ -73,12 +73,14 @@ BOOST_AUTO_TEST_CASE(login_test) { c->set_session_data( wsc ); }); - server->listen( 8090 ); + server->listen( 0 ); + auto listen_port = server->get_listening_port(); server->start_accept(); try { auto client = std::make_shared(); - auto con = client->connect( "ws://localhost:8090" ); + auto con = client->connect( "ws://localhost:" + std::to_string(listen_port) ); + server->stop_listening(); auto apic = std::make_shared(con, MAX_DEPTH); auto remote_login_api = apic->get_remote_api(); auto remote_calc = remote_login_api->get_calc(); @@ -87,8 +89,10 @@ BOOST_AUTO_TEST_CASE(login_test) { BOOST_CHECK_EQUAL(remote_calc->add( 4, 5 ), 9); BOOST_CHECK(remote_triggered); + client->synchronous_close(); + server->synchronous_close(); + fc::usleep(fc::milliseconds(50)); client.reset(); - fc::usleep(fc::milliseconds(100)); server.reset(); } FC_LOG_AND_RETHROW() } FC_LOG_AND_RETHROW() @@ -110,12 +114,14 @@ BOOST_AUTO_TEST_CASE(optionals_test) { c->set_session_data( wsc ); }); - server->listen( 8090 ); + server->listen( 0 ); + auto listen_port = server->get_listening_port(); server->start_accept(); try { auto client = std::make_shared(); - auto con = client->connect( "ws://localhost:8090" ); + auto con = client->connect( "ws://localhost:" + std::to_string(listen_port) ); + server->stop_listening(); auto apic = std::make_shared(*con, MAX_DEPTH); auto remote_optionals = apic->get_remote_api(); @@ -124,8 +130,10 @@ BOOST_AUTO_TEST_CASE(optionals_test) { BOOST_CHECK_EQUAL(remote_optionals->foo("a", "b", "c"), "[\"a\",\"b\",\"c\"]"); BOOST_CHECK_EQUAL(remote_optionals->foo("a", {}, "c"), "[\"a\",null,\"c\"]"); + client->synchronous_close(); + server->synchronous_close(); + fc::usleep(fc::milliseconds(50)); client.reset(); - fc::usleep(fc::milliseconds(100)); server.reset(); } FC_LOG_AND_RETHROW() } FC_LOG_AND_RETHROW() From 09e4f573ce2424321a9738761dfa11139f15b467 Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Sun, 5 May 2019 10:15:01 -0500 Subject: [PATCH 06/10] Improve websocket server close code --- src/network/http/websocket.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/network/http/websocket.cpp b/src/network/http/websocket.cpp index 75b882b..ff56e0a 100644 --- a/src/network/http/websocket.cpp +++ b/src/network/http/websocket.cpp @@ -207,6 +207,8 @@ namespace fc { namespace http { _server_thread.async( [&](){ auto new_con = std::make_shared>( _server.get_con_from_hdl(hdl) ); _on_connection( _connections[hdl] = new_con ); + if ( !_closed ) + _closed = new fc::promise(); }).wait(); }); _server.set_message_handler( [&]( connection_hdl hdl, websocket_server_type::message_ptr msg ){ @@ -290,9 +292,6 @@ namespace fc { namespace http { if( _server.is_listening() ) _server.stop_listening(); - if( _connections.size() ) - _closed = new fc::promise(); - auto cpy_con = _connections; for( auto item : cpy_con ) _server.close( item.first, 0, "server exit" ); @@ -654,8 +653,8 @@ namespace fc { namespace http { void websocket_server::synchronous_close() { close(); - while (!my->_connections.empty()) - fc::yield(); + if (my->_closed) + my->_closed->wait(); } From b8a03d3a51bc7a81dc57af87fbfd0fd2bf1911bf Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Mon, 6 May 2019 13:19:24 -0500 Subject: [PATCH 07/10] Fix failing test --- src/network/http/websocket.cpp | 3 +- tests/network/http/websocket_test.cpp | 53 +++++---------------------- 2 files changed, 10 insertions(+), 46 deletions(-) diff --git a/src/network/http/websocket.cpp b/src/network/http/websocket.cpp index ff56e0a..be29768 100644 --- a/src/network/http/websocket.cpp +++ b/src/network/http/websocket.cpp @@ -207,7 +207,7 @@ namespace fc { namespace http { _server_thread.async( [&](){ auto new_con = std::make_shared>( _server.get_con_from_hdl(hdl) ); _on_connection( _connections[hdl] = new_con ); - if ( !_closed ) + if ( !_closed || _closed->ready() ) _closed = new fc::promise(); }).wait(); }); @@ -216,7 +216,6 @@ namespace fc { namespace http { auto current_con = _connections.find(hdl); assert( current_con != _connections.end() ); wdump(("server")(msg->get_payload())); - //std::cerr<<"recv: "<get_payload()<<"\n"; auto payload = msg->get_payload(); std::shared_ptr con = current_con->second; ++_pending_messages; diff --git a/tests/network/http/websocket_test.cpp b/tests/network/http/websocket_test.cpp index 10068b0..8c726e5 100644 --- a/tests/network/http/websocket_test.cpp +++ b/tests/network/http/websocket_test.cpp @@ -20,21 +20,8 @@ BOOST_AUTO_TEST_CASE(websocket_test) }); }); - bool listen_ok = false; - for( int i = 0; !listen_ok && i < 5; ++i ) - { - port = std::rand() % 50000 + 10000; - try - { - server.listen( port ); - listen_ok = true; - } - catch( std::exception& ignore ) - { - // if the port is busy, listen() will throw a std::exception, do nothing here. - } - } - BOOST_REQUIRE( listen_ok ); + server.listen( 0 ); + port = server.get_listening_port(); server.start_accept(); @@ -44,49 +31,27 @@ BOOST_AUTO_TEST_CASE(websocket_test) echo = s; }); c_conn->send_message( "hello world" ); - fc::usleep( fc::seconds(1) ); + fc::usleep( fc::milliseconds(100) ); BOOST_CHECK_EQUAL("echo: hello world", echo); c_conn->send_message( "again" ); - fc::usleep( fc::seconds(1) ); + fc::usleep( fc::milliseconds(100) ); BOOST_CHECK_EQUAL("echo: again", echo); s_conn->close(0, "test"); - fc::usleep( fc::seconds(1) ); - try { - c_conn->send_message( "again" ); - BOOST_FAIL("expected assertion failure"); - } catch (const fc::exception& e) { - //std::cerr << e.to_string() << "\n"; - } + fc::usleep( fc::milliseconds(100) ); + BOOST_CHECK_THROW(c_conn->send_message( "again" ), fc::exception); c_conn = client.connect( "ws://localhost:" + fc::to_string(port) ); c_conn->on_message_handler([&](const std::string& s){ echo = s; }); c_conn->send_message( "hello world" ); - fc::usleep( fc::seconds(1) ); + fc::usleep( fc::milliseconds(100) ); BOOST_CHECK_EQUAL("echo: hello world", echo); } - try { - c_conn->send_message( "again" ); - BOOST_FAIL("expected assertion failure"); - } catch (const fc::assert_exception& e) { - std::cerr << "Expected assertion failure : " << e.to_string() << "\n"; - } catch (const fc::exception& e) { - BOOST_FAIL("Unexpected exception : " + e.to_string()); - } catch (const std::exception& e) { - BOOST_FAIL("Unexpected exception : " + std::string(e.what())); - } - - try { - c_conn = client.connect( "ws://localhost:" + fc::to_string(port) ); - BOOST_FAIL("expected fc::exception (fail to connect)"); - } catch (const fc::exception& e) { - std::cerr << "Excepted fc::exception : " << e.to_string() << "\n"; - } catch (const std::exception& e) { - BOOST_FAIL("Unexpected exception : " + std::string(e.what())); - } + BOOST_CHECK_THROW(c_conn->send_message( "again" ), fc::assert_exception); + BOOST_CHECK_THROW(client.connect( "ws://localhost:" + fc::to_string(port) ), fc::exception); } BOOST_AUTO_TEST_SUITE_END() From 0a50ac23e0eb3381accf48f820f9e3e4a8798213 Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Tue, 7 May 2019 07:55:40 -0500 Subject: [PATCH 08/10] Remove websocket_server::synchronous_close It doesn't work as expected, so get rid of it. --- include/fc/network/http/websocket.hpp | 1 - src/network/http/websocket.cpp | 8 -------- tests/api_tests.cpp | 4 ++-- 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/include/fc/network/http/websocket.hpp b/include/fc/network/http/websocket.hpp index 8225823..11db65c 100644 --- a/include/fc/network/http/websocket.hpp +++ b/include/fc/network/http/websocket.hpp @@ -55,7 +55,6 @@ namespace fc { namespace http { void stop_listening(); void close(); - void synchronous_close(); private: friend class detail::websocket_server_impl; diff --git a/src/network/http/websocket.cpp b/src/network/http/websocket.cpp index be29768..dbff4bc 100644 --- a/src/network/http/websocket.cpp +++ b/src/network/http/websocket.cpp @@ -649,14 +649,6 @@ namespace fc { namespace http { my->_server.close(connection.first, websocketpp::close::status::normal, "Goodbye"); } - void websocket_server::synchronous_close() - { - close(); - if (my->_closed) - my->_closed->wait(); - } - - websocket_tls_server::websocket_tls_server( const string& server_pem, const string& ssl_password ):my( new detail::websocket_tls_server_impl(server_pem, ssl_password) ) {} diff --git a/tests/api_tests.cpp b/tests/api_tests.cpp index 00997ad..9f9b3a4 100644 --- a/tests/api_tests.cpp +++ b/tests/api_tests.cpp @@ -90,7 +90,7 @@ BOOST_AUTO_TEST_CASE(login_test) { BOOST_CHECK(remote_triggered); client->synchronous_close(); - server->synchronous_close(); + server->close(); fc::usleep(fc::milliseconds(50)); client.reset(); server.reset(); @@ -131,7 +131,7 @@ BOOST_AUTO_TEST_CASE(optionals_test) { BOOST_CHECK_EQUAL(remote_optionals->foo("a", {}, "c"), "[\"a\",null,\"c\"]"); client->synchronous_close(); - server->synchronous_close(); + server->close(); fc::usleep(fc::milliseconds(50)); client.reset(); server.reset(); From afb96a0e7ebc9bd6569af97888d6e8ef59f418f5 Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Wed, 8 May 2019 12:09:44 -0500 Subject: [PATCH 09/10] Ref #126: Cleanup/revert unwanted changes --- include/fc/api.hpp | 3 ++ src/network/http/websocket.cpp | 5 +-- tests/api_tests.cpp | 62 ++++++++++++++++------------------ 3 files changed, 35 insertions(+), 35 deletions(-) diff --git a/include/fc/api.hpp b/include/fc/api.hpp index d75a159..5e582ad 100644 --- a/include/fc/api.hpp +++ b/include/fc/api.hpp @@ -28,6 +28,9 @@ namespace fc { /// whereas normally the last two arguments must be provided, this template allows them to be omitted. /// Note that this only applies to trailing optional arguments, i.e. given a callable taking /// (fc::optional, int, fc::optional), only the last argument can be omitted. + /// + /// A discussion of how exactly this works is available here: + /// https://github.com/bitshares/bitshares-fc/pull/126#issuecomment-490566060 template struct optionals_callable : public std::function { using std::function::operator(); diff --git a/src/network/http/websocket.cpp b/src/network/http/websocket.cpp index dbff4bc..6d479bf 100644 --- a/src/network/http/websocket.cpp +++ b/src/network/http/websocket.cpp @@ -207,8 +207,6 @@ namespace fc { namespace http { _server_thread.async( [&](){ auto new_con = std::make_shared>( _server.get_con_from_hdl(hdl) ); _on_connection( _connections[hdl] = new_con ); - if ( !_closed || _closed->ready() ) - _closed = new fc::promise(); }).wait(); }); _server.set_message_handler( [&]( connection_hdl hdl, websocket_server_type::message_ptr msg ){ @@ -291,6 +289,9 @@ namespace fc { namespace http { if( _server.is_listening() ) _server.stop_listening(); + if ( _connections.size() ) + _closed = new fc::promise(); + auto cpy_con = _connections; for( auto item : cpy_con ) _server.close( item.first, 0, "server exit" ); diff --git a/tests/api_tests.cpp b/tests/api_tests.cpp index 9f9b3a4..5c1147c 100644 --- a/tests/api_tests.cpp +++ b/tests/api_tests.cpp @@ -77,24 +77,22 @@ BOOST_AUTO_TEST_CASE(login_test) { auto listen_port = server->get_listening_port(); server->start_accept(); - try { - auto client = std::make_shared(); - auto con = client->connect( "ws://localhost:" + std::to_string(listen_port) ); - server->stop_listening(); - auto apic = std::make_shared(con, MAX_DEPTH); - auto remote_login_api = apic->get_remote_api(); - auto remote_calc = remote_login_api->get_calc(); - bool remote_triggered = false; - remote_calc->on_result( [&remote_triggered]( uint32_t r ) { remote_triggered = true; } ); - BOOST_CHECK_EQUAL(remote_calc->add( 4, 5 ), 9); - BOOST_CHECK(remote_triggered); + auto client = std::make_shared(); + auto con = client->connect( "ws://localhost:" + std::to_string(listen_port) ); + server->stop_listening(); + auto apic = std::make_shared(con, MAX_DEPTH); + auto remote_login_api = apic->get_remote_api(); + auto remote_calc = remote_login_api->get_calc(); + bool remote_triggered = false; + remote_calc->on_result( [&remote_triggered]( uint32_t r ) { remote_triggered = true; } ); + BOOST_CHECK_EQUAL(remote_calc->add( 4, 5 ), 9); + BOOST_CHECK(remote_triggered); - client->synchronous_close(); - server->close(); - fc::usleep(fc::milliseconds(50)); - client.reset(); - server.reset(); - } FC_LOG_AND_RETHROW() + client->synchronous_close(); + server->close(); + fc::usleep(fc::milliseconds(50)); + client.reset(); + server.reset(); } FC_LOG_AND_RETHROW() } @@ -118,24 +116,22 @@ BOOST_AUTO_TEST_CASE(optionals_test) { auto listen_port = server->get_listening_port(); server->start_accept(); - try { - auto client = std::make_shared(); - auto con = client->connect( "ws://localhost:" + std::to_string(listen_port) ); - server->stop_listening(); - auto apic = std::make_shared(*con, MAX_DEPTH); - auto remote_optionals = apic->get_remote_api(); + auto client = std::make_shared(); + auto con = client->connect( "ws://localhost:" + std::to_string(listen_port) ); + server->stop_listening(); + auto apic = std::make_shared(*con, MAX_DEPTH); + auto remote_optionals = apic->get_remote_api(); - BOOST_CHECK_EQUAL(remote_optionals->foo("a"), "[\"a\",null,null]"); - BOOST_CHECK_EQUAL(remote_optionals->foo("a", "b"), "[\"a\",\"b\",null]"); - BOOST_CHECK_EQUAL(remote_optionals->foo("a", "b", "c"), "[\"a\",\"b\",\"c\"]"); - BOOST_CHECK_EQUAL(remote_optionals->foo("a", {}, "c"), "[\"a\",null,\"c\"]"); + BOOST_CHECK_EQUAL(remote_optionals->foo("a"), "[\"a\",null,null]"); + BOOST_CHECK_EQUAL(remote_optionals->foo("a", "b"), "[\"a\",\"b\",null]"); + BOOST_CHECK_EQUAL(remote_optionals->foo("a", "b", "c"), "[\"a\",\"b\",\"c\"]"); + BOOST_CHECK_EQUAL(remote_optionals->foo("a", {}, "c"), "[\"a\",null,\"c\"]"); - client->synchronous_close(); - server->close(); - fc::usleep(fc::milliseconds(50)); - client.reset(); - server.reset(); - } FC_LOG_AND_RETHROW() + client->synchronous_close(); + server->close(); + fc::usleep(fc::milliseconds(50)); + client.reset(); + server.reset(); } FC_LOG_AND_RETHROW() } From 6b7874e49ade49d087ddd3cadb51bcc685ebcbe9 Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Mon, 13 May 2019 17:26:32 -0500 Subject: [PATCH 10/10] Rebase fixes --- tests/api_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/api_tests.cpp b/tests/api_tests.cpp index 5c1147c..0902492 100644 --- a/tests/api_tests.cpp +++ b/tests/api_tests.cpp @@ -107,7 +107,7 @@ BOOST_AUTO_TEST_CASE(optionals_test) { auto server = std::make_shared(); server->on_connection([&]( const websocket_connection_ptr& c ){ - auto wsc = std::make_shared(*c, MAX_DEPTH); + auto wsc = std::make_shared(c, MAX_DEPTH); wsc->register_api(fc::api(optionals)); c->set_session_data( wsc ); }); @@ -119,7 +119,7 @@ BOOST_AUTO_TEST_CASE(optionals_test) { auto client = std::make_shared(); auto con = client->connect( "ws://localhost:" + std::to_string(listen_port) ); server->stop_listening(); - auto apic = std::make_shared(*con, MAX_DEPTH); + auto apic = std::make_shared(con, MAX_DEPTH); auto remote_optionals = apic->get_remote_api(); BOOST_CHECK_EQUAL(remote_optionals->foo("a"), "[\"a\",null,null]");