fix #514, call_data_callbacks after shared the data

[jmorice91]

!!!INSERT YOUR DESCRIPTION HERE!!!

List of things to check before making a PR

Before merging your code, please check the following:

• you have added a line describing your changes to the Changelog;
• you have added unit tests for any new or improved feature;
• In case you updated dependencies, you have checked pdi/docs/CheckList.md
• you have checked your code format:
  • you have checked that you respect all conventions specified in CONTRIBUTING.md;
  • you have checked that the indentation and formatting conforms to the .clang-format;
  • you have documented with doxygen any new or changed function / class;
• you have correctly updated the copyright headers:
  • your institution is in the copyright header of every file you (substantially) modified;
  • you have checked that the end-year of the copyright there is the current one;
• you have updated the AUTHORS file:
  • you have added yourself to the AUTHORS file;
  • if this is a new contribution, you have added it to the AUTHORS file;
• you have added everything to the user documentation:
  • any new CMake configuration option;
  • any change in the yaml config;
  • any change to the public or plugin API;
  • any other new or changed user-facing feature;
  • any change to the dependencies;
• you have correctly linked your MR to one or more issues:
  • your MR solves an identified issue;
  • your commit contain the Fix #issue keyword to autoclose the issue when merged.

[benoitmartin88]

Great feature to add. Thanks !
Not much is needed to merge this PR.
I think this feature would require a unit test. Could you add this?

[benoitmartin88]

Almost ready to merge.
I do have a few questions that need answering concerning error handling, potentially superfluous functions and writing tests in C++.

[benoitmartin88]

I think this is almost ready to merge.
Could you make sure the indent and spacing of the modified files is correct ?
Thanks !

[jmorice91]

I think this is almost ready to merge. Could you make sure the indent and spacing of the modified files is correct ? Thanks !

Is it normal that the action indent doesn't catch that?

[Yushan-Wang]

Ready to merge

[Yushan-Wang]

@jbigot Could you review this PR please?

[jbigot]

This is a great submission! My main comment is about moving Var_to_reclaim of the provider side instead of expecting each user to implement it. Overall this is really nice though!

[jbigot]

What's the status for this one?

[jmorice91]

What's the status for this one?

It is ready to review.

[jbigot]

@Yushan-Wang , @benoitmartin88 , @JAuriac : I'd like a cross review first and then I'll do one. Who has time for one?

[benoitmartin88]

Je regarde.

[benoitmartin88]

Thank you for this contribution. There are a few changes that are required.

[Yushan-Wang]

remove?

[jmorice91]

I will be remove in the next commit

[jbigot]

Suggested change

// #include "global_context.h"

[jbigot]

Suggested change

	* \param delayed_callbacks a class that allow to delay trigger_delayed_data_callbacks for a list of a data
	* \param delayed_callbacks a list of callbacks where the callback for this data will be added, instead of being triggered. So that one can delay the trigger

[jbigot]

Suggested change

	* \param delayed_callbacks a class that allow to delay trigger_delayed_data_callbacks for a list of a data
	* \param delayed_callbacks a list of callbacks where the callback for this data will be added, instead of being triggered. So that one can delay the trigger

[jbigot]

Why is it still required?

[jmorice91]

We need that because m_context[element_name.c_str()] is a data_descriptor in Delayed_data_callbacks

Do you want to remove trigger_delayed_data_callbacks() in the class data_descriptor_impl and put this function inside class PDI_EXPORT Delayed_data_callbacks?

I want to keep this function inside data_descriptor_impl because I don't want to change m_refs outside this class

/// References to the values of this descriptor
std::stack<std::unique_ptr<Ref_holder>> m_refs;

[jbigot]

What does this comment mean?

[jbigot]

since you rethrow. I wouldn't print anything in that case:

Suggested change

m_context.logger().error("Error in the destructor of Delayed_data_callbacks.");

[jbigot]

same as above

Suggested change

m_context.logger().error("Error in the destructor of Delayed_data_callbacks.");

[jbigot]

why not move the try one step up?

[jbigot]

When an exception is thrown, we'd like the events not yet triggered to remain in the list of events to trigger.

Suggested change

	void trigger()
	try {
	int i = 0;
	size_t number_of_elements = m_datanames.size();
	for (auto&& element_name: m_datanames) {
	m_context.logger().trace("Trigger data callback `{}' ({}/{})", element_name.c_str(), ++i, number_of_elements);
	m_context[element_name.c_str()].trigger_delayed_data_callbacks();
	}
	this->cancel(); // clear the m_datanames
	} catch (Error& e) {
	this->cancel(); // clear the m_datanames
	throw Error(e.status(), "Error in trigger data callback: `{}'", e.what());
	} catch (...) {
	this->cancel(); // clear the m_datanames
	throw;
	}
	void trigger()
	{
	int i = 0;
	size_t number_of_elements = m_datanames.size();
	while ( ! m_datanames.empty() ) {
	auto&& element_name = m_datanames.back();
	m_context.logger().trace("Trigger data callback `{}' ({}/{})", element_name, ++i, number_of_elements);
	m_context[element_name].trigger_delayed_data_callbacks();
	m_datanames.pop_back()
	}
	}

[jbigot]

Why is this class still required? Isn't Delayed_data_callbacks enough?

[jmorice91]

These two classes are defined with the same list of dataname in some sense.
(It depends only if we have an error after delayed_callbacks.add_dataname(m_name);
in void* Data_descriptor_impl::share) -> good catch.

Remark:
In PDI_share, the destruction of the object Delayed_data_callbacks execute event on_data in this implementation.
The class Var_to_reclaim is used to call PDI_reclaim the data in PDI_multi_expose.

So if we want to have only 1 class:

• need to rename the class Delayed_data_callbacks -> name ???
• add a member to this class to know if we are in the multi_expose case to have two version of destructor:

1. dtor1 : call trigger()
2. dtor2: call trigger() + reclaim the variables.

• Perhaps add a member to this class to know if we call trigger() before.
• don't remove element in the list of the dataname in trigger()

Is it what you want to implement? we can discuss if you want.