Complete structure for BackEnd RPC services #601

canepat · 2022-03-04T12:33:57Z

Add server-streaming RPC support
Complete test client for for BackEnd RPCs

Complete structure for BackEnd RPC services Complete test client for for BackEnd RPCs

codecov · 2022-03-04T12:55:37Z

Codecov Report

Merging #601 (79bd762) into master (7daf5b8) will increase coverage by 0.21%.
The diff coverage is 97.46%.

@@            Coverage Diff             @@
##           master     #601      +/-   ##
==========================================
+ Coverage   81.55%   81.77%   +0.21%     
==========================================
  Files         169      169              
  Lines       13768    13954     +186     
==========================================
+ Hits        11229    11411     +182     
- Misses       2539     2543       +4

Impacted Files	Coverage Δ
node/silkworm/rpc/call.hpp	`95.53% <95.00%> (-0.68%)`	⬇️
node/silkworm/rpc/backend_server.cpp	`100.00% <100.00%> (ø)`
node/silkworm/rpc/backend_server.hpp	`100.00% <100.00%> (ø)`
node/silkworm/rpc/server_config.cpp	`78.57% <100.00%> (+5.84%)`	⬆️
node/silkworm/rpc/server_config.hpp	`100.00% <100.00%> (ø)`
node/silkworm/rpc/service.hpp	`100.00% <100.00%> (ø)`
core/silkworm/consensus/base/engine.cpp	`93.03% <0.00%> (-1.00%)`	⬇️
core/silkworm/trie/hash_builder.cpp	`99.29% <0.00%> (-0.71%)`	⬇️
core/silkworm/state/in_memory_state.cpp	`94.14% <0.00%> (+0.48%)`	⬆️
core/silkworm/execution/evm.cpp	`96.90% <0.00%> (+0.82%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7daf5b8...79bd762. Read the comment docs.

Add server config unit tests Fix race condition in server unit test

AndreaLanfranchi · 2022-03-06T22:06:30Z

node/silkworm/rpc/backend_server.cpp

@@ -58,8 +58,93 @@ void NetVersionService::process_rpc(NetVersionUnaryRpc& rpc, const remote::NetVe
    SILK_TRACE << "NetVersionService::process_rpc rsp: " << &response << " chain_id: " << chain_id_ << " sent: " << sent;
 }

+void NetPeerCountService::process_rpc(NetPeerCountRpc& rpc, const remote::NetPeerCountRequest* request) {
+    SILK_TRACE << "NetPeerCountService::process_rpc rpc: " << &rpc << " request: " << request;


Why print reference to rpc ? I mean << &rpc ?

Also ... do rpc and rsp stand for request and response ?
And again ... out of curiosity why don't write

SILK_TRACE << __FUNCTION__ << " rpc: " << &rpc << " request: " << request;

At least, should you change function naming, don't have to rewrite in the log lines

Why print reference to rpc ? I mean << &rpc ?

I'm printing the memory address of rpc for tracing purposes, it is useful to debug RPC lifetime issues (live RPC objects are allocated and maintained on the heap, then deallocated on cleanup)

Also ... do rpc and rsp stand for request and response ?

rpc stands for remote procedure call, but you have a point here: naming is too much close between rpc and rsp in the trace log, I will amend it.

And again ... out of curiosity why don't write

SILK_TRACE << __FUNCTION__ << " rpc: " << &rpc << " request: " << request;

At least, should you change function naming, don't have to rewrite in the log lines

Yes, you're right, definitely better. Maybe we can go one step further and include __FUNCTION__ into a couple of extended SILK_TRACE logging macros to avoid the repetition altogether:

#define SILK_TRACE_ENTER SILK_TRACE << __FUNCTION__ << " ENTER" #define SILK_TRACE_EXIT SILK_TRACE << __FUNCTION__ << " EXIT"

or

#define SILK_TRACE_START SILK_TRACE << __FUNCTION__ << " START" #define SILK_TRACE_END SILK_TRACE << __FUNCTION__ << " END"

Please don't ... 🙏
Don't add another macro 😄

Hmmm ... I don't know on linux but at least on windows __FUNCTION__ returns a FQDN of the function like in this example

ERROR [03-05|19:15:51.470 UTC] HashState function=silkworm::stagedsync::HashState::hash_from_account_changeset exception=kAborted

The problem here is probably that __FUNCTION__ is not standard (neither are the other options unfortunately)

MSVC 1 - GCC 0

The problem here is probably that __FUNCTION__ is not standard (neither are the other options unfortunately)

maybe use this https://www.boost.org/doc/libs/1_38_0/libs/utility/current_function.html. Apparently it is a small header that doesn't depend on any other header.

the whole header, or how to get the current function name in any compiler.

#ifndef BOOST_CURRENT_FUNCTION_HPP_INCLUDED #define BOOST_CURRENT_FUNCTION_HPP_INCLUDED // MS compatible compilers support #pragma once #if defined(_MSC_VER) && (_MSC_VER >= 1020) # pragma once #endif // // boost/current_function.hpp - BOOST_CURRENT_FUNCTION // // Copyright (c) 2002 Peter Dimov and Multi Media Ltd. // // Distributed under the Boost Software License, Version 1.0. // See accompanying file LICENSE_1_0.txt or copy at // http://www.boost.org/LICENSE_1_0.txt // // [http://www.boost.org/libs/assert/current_function.html](https://www.boost.org/doc/libs/1_63_0/libs/assert/current_function.html) // namespace boost { namespace detail { inline void current_function_helper() { #if defined( BOOST_DISABLE_CURRENT_FUNCTION ) # define BOOST_CURRENT_FUNCTION "(unknown)" #elif defined(__GNUC__) || (defined(__MWERKS__) && (__MWERKS__ >= 0x3000)) || (defined(__ICC) && (__ICC >= 600)) || defined(__ghs__) # define BOOST_CURRENT_FUNCTION __PRETTY_FUNCTION__ #elif defined(__DMC__) && (__DMC__ >= 0x810) # define BOOST_CURRENT_FUNCTION __PRETTY_FUNCTION__ #elif defined(__FUNCSIG__) # define BOOST_CURRENT_FUNCTION __FUNCSIG__ #elif (defined(__INTEL_COMPILER) && (__INTEL_COMPILER >= 600)) || (defined(__IBMCPP__) && (__IBMCPP__ >= 500)) # define BOOST_CURRENT_FUNCTION __FUNCTION__ #elif defined(__BORLANDC__) && (__BORLANDC__ >= 0x550) # define BOOST_CURRENT_FUNCTION __FUNC__ #elif defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901) # define BOOST_CURRENT_FUNCTION __func__ #elif defined(__cplusplus) && (__cplusplus >= 201103) # define BOOST_CURRENT_FUNCTION __func__ #else # define BOOST_CURRENT_FUNCTION "(unknown)" #endif } } // namespace detail } // namespace boost

#endif // #ifndef BOOST_CURRENT_FUNCTION_HPP_INCLUDED

@greg7mdp thanks for your suggestion, but for this use case (including class::method in a log trace) BOOST_CURRENT_FUNCTION has the same drawback as __PRETTY_FUNCTION__ (obviously, because it is the same at least for GCC): too much verbose.

I think that sticking to the hard-coded class::method names in few log traces is not so bad, anyway.

mriccobene · 2022-03-07T12:13:14Z

IMO the readability of the class RpcService can be improved:

template <
    typename AsyncService,
    typename Request,
    typename Reply,
    template<typename, typename, typename> typename GenericRpc> // <= note the new name here
class RpcService {
    using Rpc = GenericRpc<AsyncService, Request, Reply>;  // <= added
    using RpcServiceHandlers = typename Rpc::Handlers;
    

[...]

    auto add(Rpc* rpc)    // <= note the method new name here
    auto remove(Rpc* rpc)  // <= note the method new name here

Also I note you are following the C++ Core Guidelines, where they say a raw pointer (T*) is non-owning . I don't like this point in the guidelines very much because it is a convention that not everyone knows and not everyone respects, so the reader of the signature, like me, do not know; moreover the compiler cannot reinforce it. I would put at least one custom attribute like [[non_owning]] but in this project they are not allowed.
But since the methods "add" and "remove" copy the rpc, is it possible to pass it for copy and then move it inside the container?

Edit: I didn't realize that "add" takes ownership of rpc so I suggest you use unique_ptr<Rpc*> or gsl::owner<Rpc*> in the signature.

mriccobene

LGTM (see my comments in the previous post).

canepat · 2022-03-07T17:27:32Z

IMO the readability of the class RpcService can be improved:

template <
    typename AsyncService,
    typename Request,
    typename Reply,
    template<typename, typename, typename> typename GenericRpc> // <= note the new name here
class RpcService {
    using Rpc = GenericRpc<AsyncService, Request, Reply>;  // <= added
    using RpcServiceHandlers = typename Rpc::Handlers;
    

[...]

    auto add(Rpc* rpc)    // <= note the method new name here
    auto remove(Rpc* rpc)  // <= note the method new name here

Sure. Adding a using directive for Rpc is a good idea.

Also I note you are following the C++ Core Guidelines, where they say a raw pointer (T*) is non-owning . I don't like this point in the guidelines very much because it is a convention that not everyone knows and not everyone respects, so the reader of the signature, like me, do not know; moreover the compiler cannot reinforce it. I would put at least one custom attribute like [[non_owning]] but in this project they are not allowed. But since the methods "add" and "remove" copy the rpc, is it possible to pass it for copy and then move it inside the container?

Edit: I didn't realize that "add" takes ownership of rpc so I suggest you use unique_ptr<Rpc*> or gsl::owner<Rpc*> in the signature.

TBH I don't know if std::unique_ptr fits well here because it could be used only for add_request and would require a double move to put the instance into the requests_ container. On the other hand, I didn't know about gsl::owner and its suggested usages: I think that also for gsl::owner your reasoning about the need for a convention to be known and respected applies, nevertheless I am happy to use it in this case if it helps understanding the code better.

Add using directive to improve template readability Rename methods to improve template readability

Add server-streaming RPC support

8ed3db3

Complete structure for BackEnd RPC services Complete test client for for BackEnd RPCs

canepat added 4 commits March 4, 2022 14:42

Add unit tests

796ee0a

Add ClientVersion RPC implementation

37ec5ef

Add server config unit tests Fix race condition in server unit test

Add unit test

0c49766

Fix data race in unit test

ddbfb0c

canepat marked this pull request as ready for review March 5, 2022 12:16

canepat requested review from AndreaLanfranchi, mriccobene and yperbasis March 5, 2022 12:16

AndreaLanfranchi reviewed Mar 6, 2022

View reviewed changes

mriccobene approved these changes Mar 7, 2022

View reviewed changes

canepat added 2 commits March 7, 2022 21:21

Merge branch 'master' into backend_kv_server2

323d269

Add gsl::owner to better express pointer ownership

79bd762

Add using directive to improve template readability Rename methods to improve template readability

canepat merged commit 03e3011 into master Mar 8, 2022

canepat deleted the backend_kv_server2 branch March 8, 2022 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete structure for BackEnd RPC services #601

Complete structure for BackEnd RPC services #601

canepat commented Mar 4, 2022

codecov bot commented Mar 4, 2022 •

edited

Loading

AndreaLanfranchi Mar 6, 2022

AndreaLanfranchi Mar 6, 2022

canepat Mar 7, 2022

canepat Mar 7, 2022

AndreaLanfranchi Mar 7, 2022

canepat Mar 7, 2022 •

edited

Loading

AndreaLanfranchi Mar 7, 2022

greg7mdp Mar 7, 2022 •

edited

Loading

greg7mdp Mar 7, 2022

canepat Mar 7, 2022

mriccobene commented Mar 7, 2022 •

edited

Loading

mriccobene left a comment •

edited

Loading

canepat commented Mar 7, 2022 •

edited

Loading

Complete structure for BackEnd RPC services #601

Complete structure for BackEnd RPC services #601

Conversation

canepat commented Mar 4, 2022

codecov bot commented Mar 4, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canepat Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg7mdp Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mriccobene commented Mar 7, 2022 • edited Loading

mriccobene left a comment • edited Loading

Choose a reason for hiding this comment

canepat commented Mar 7, 2022 • edited Loading

codecov bot commented Mar 4, 2022 •

edited

Loading

canepat Mar 7, 2022 •

edited

Loading

greg7mdp Mar 7, 2022 •

edited

Loading

mriccobene commented Mar 7, 2022 •

edited

Loading

mriccobene left a comment •

edited

Loading

canepat commented Mar 7, 2022 •

edited

Loading