Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

valgrind inspired fixes #1693

Merged
merged 4 commits into from
Apr 10, 2019
Merged

valgrind inspired fixes #1693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8ce31a
fix referencing local stack variable in async thread
crypto-ape Apr 2, 2019
376f031
explicitly cleanup external library facilities
crypto-ape Apr 2, 2019
53af67e
fix referencing local stack variable in async thread // add missing p…
crypto-ape Apr 2, 2019
3ab22f6
fix referencing local stack variable in async thread // capture share…
crypto-ape Apr 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions libraries/net/node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4909,7 +4909,8 @@ namespace graphene { namespace net { namespace detail {
# define INVOKE_AND_COLLECT_STATISTICS(method_name, ...) \
try \
{ \
call_statistics_collector statistics_collector(#method_name, \
std::shared_ptr<call_statistics_collector> statistics_collector = std::make_shared<call_statistics_collector>( \
#method_name, \
&_ ## method_name ## _execution_accumulator, \
&_ ## method_name ## _delay_before_accumulator, \
&_ ## method_name ## _delay_after_accumulator); \
Expand All @@ -4919,7 +4920,7 @@ namespace graphene { namespace net { namespace detail {
return _node_delegate->method_name(__VA_ARGS__); \
} \
else \
return _thread->async([&](){ \
return _thread->async([&, statistics_collector](){ \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the & and capture this instead. Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry, but this won't work.

Strictly speaking, it would be enough to capture by copy only this->_node_delegate. But omitting the implicit by-reference capture (i.e. &) the __VA_ARGS__ won't be captured.

Well, the simplest solution is to use the implicit capture by copy (i.e. =), which won't compile because the functions used with the macro consume parameters as references (see the interface class node_delegate), at least one uses such parameter as an additional output parameter.

Furthermore, it is not easily seen that the original caller provides these references as of non-stack allocated entities. Thus, to be completely bug-proof, all of these parameters shall be passed in shared_ptr.

Now, from the valgrind output I have produced, it does not seem this is the case. I suppose the proposed modification to be accepted as is (it provably solves an existing problem), and in the case a strictly complete solution is deemed required, a specific issue to be opened for the required refactor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed the VA_ARGS, sorry.

handle_message, for example, uses a stack-allocated message (originating from message_oriented_connection_impl::read_loop). Normally this lives longer than the handle_message call should take, however, it is re-used when reading the next message (which could lead to data corruption), and it is destroyed when reading fails (e. g. when the connection breaks down). This could even be related to #1256 @jmjatlanta ?

I agree that this should be handled in another ticket.

call_statistics_collector::actual_execution_measurement_helper helper(statistics_collector); \
return _node_delegate->method_name(__VA_ARGS__); \
}, "invoke " BOOST_STRINGIZE(method_name)).wait(); \
Expand All @@ -4941,7 +4942,8 @@ namespace graphene { namespace net { namespace detail {
}
#else
# define INVOKE_AND_COLLECT_STATISTICS(method_name, ...) \
call_statistics_collector statistics_collector(#method_name, \
std::shared_ptr<call_statistics_collector> statistics_collector = std::make_shared<call_statistics_collector>( \
#method_name, \
&_ ## method_name ## _execution_accumulator, \
&_ ## method_name ## _delay_before_accumulator, \
&_ ## method_name ## _delay_after_accumulator); \
Expand All @@ -4951,7 +4953,7 @@ namespace graphene { namespace net { namespace detail {
return _node_delegate->method_name(__VA_ARGS__); \
} \
else \
return _thread->async([&](){ \
return _thread->async([&, statistics_collector](){ \
call_statistics_collector::actual_execution_measurement_helper helper(statistics_collector); \
return _node_delegate->method_name(__VA_ARGS__); \
}, "invoke " BOOST_STRINGIZE(method_name)).wait()
Expand Down
8 changes: 4 additions & 4 deletions libraries/net/node_impl.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,16 @@ private:
public:
class actual_execution_measurement_helper
{
call_statistics_collector &_collector;
std::shared_ptr<call_statistics_collector> _collector;
public:
actual_execution_measurement_helper(call_statistics_collector& collector) :
actual_execution_measurement_helper(std::shared_ptr<call_statistics_collector> collector) :
_collector(collector)
{
_collector.starting_execution();
_collector->starting_execution();
}
~actual_execution_measurement_helper()
{
_collector.execution_completed();
_collector->execution_completed();
}
};
call_statistics_collector(const char* method_name,
Expand Down
4 changes: 4 additions & 0 deletions libraries/plugins/elasticsearch/elasticsearch_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ class elasticsearch_plugin_impl

elasticsearch_plugin_impl::~elasticsearch_plugin_impl()
{
if (curl) {
curl_easy_cleanup(curl);
curl = nullptr;
}
return;
}

Expand Down
4 changes: 4 additions & 0 deletions libraries/plugins/es_objects/es_objects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,10 @@ void es_objects_plugin_impl::prepareTemplate(T blockchain_object, string index_n

es_objects_plugin_impl::~es_objects_plugin_impl()
{
if (curl) {
curl_easy_cleanup(curl);
curl = nullptr;
}
return;
}

Expand Down