When testing a cycle event, calling application_impl::stop_offer_event is invalid. #666

[hjkcst]

When testing a cycle event, calling application_impl::stop_offer_event is invalid. #666

See bug#666 for details of the issue.

[007herelsp]

I think this modification is dangerous,

[hjkcst]

Do you have a better way to solve this problem?

[hjkcst]

Does anyone know who is required to review and merge code after committing changes? What is the process like?

[007herelsp]

@hjkcst Can you provide the test code

[hjkcst]

@hjkcst Can you provide the test code
Sorry, the specific code may not be convenient to disclose, you can refer to the test process in this issue, run the stop_offer_event call, you can see the problem.
#666

[007herelsp]

void event::update_cbk(boost::system::error_code const &_error) added the is_provided_ detection, which is used to exit the cycle_timer

[hjkcst]

void event::update_cbk(boost::system::error_code const &_error) added the is_provided_ detection, which is used to exit the cycle_timer

Hello,007herelsp

I think this modification cannot completely delete the reference count of the pointer corresponding to cycle event, which means that cycle event instances still exist. The function of application_impl::stop_offer_event is to unregister the cycle event, that is, to delete it rather than stop it.

thanks,jinkuan

[007herelsp]

implementation/routing/src/routing_manager_base.cpp

implementation/routing/src/event.cpp

There is deletion logic

[hjkcst]

implementation/routing/src/routing_manager_base.cpp implementation/routing/src/event.cpp

There is deletion logic

Hello,007herelsp

I know this is delete logic, I mean not completely delete clean (this has to do with the nature of the smart pointer). Perhaps you can test this by printing the reference count of the cycle event by adding a log at the end of the deletion.
For example, ptr1.use_count ().

thanks,jinkuan

[007herelsp]

implementation/routing/src/routing_manager_base.cpp

add its_event->set_provided(false);

and void event::update_cbk(boost::system::error_code const &error) added the is_provided detection, which is used to exit the cycle_timer

I have tested this code and it can successfully release the event data, but I am not sure if it brings other problems, you can test it

[hjkcst]

I think it needs to be tested by the authorities, so how can the authorities intervene in this modification?

[hjkcst]

halo, administrator of the vsomeip library, pay attention to this issue.

[duartenfonseca]

hi @hjkcst @007herelsp i am trying to reproduce your problem. for that i used the following network_test: cyclic_event_test.
i did a change to add a stop_offer_event after the notify of the service.

i wasn't able to see the same problem. can you point out some code for the example, or why in my case it didn't work?

[hjkcst]

@duartenfonseca
What you are testing is not a cycle event.
ref:
app->offer_event(serviceId, instanceId,
eventId, groups, vsomeip::event_type_e::ET_EVENT,
std::chrono::milliseconds(cycleTime));

[hjkcst]

@007herelsp Regarding this issue, I have a new understanding. After testing, it was successfully possible to stop the event notification.However, it is impossible to assess whether such modifications comply with the regulations of the SomeIP protocol.

Could you please tell me if there is any way to get the community workers' attention to this issue?

[007herelsp]

@hjkcst
I don't think this modification will affect the SOME/IP protocol.
Sorry, I also don't have any good way to attract the attention of community workers.
It seems that community workers don't really care about the issues raised here.

[duartenfonseca]

@hjkcst @007herelsp i am from the community. some questions:

• this only happens with 3.4.10? what about 3.5.5?
• i have tried with your correction, using ET_EVENT, and still it didn't happen.

Here is a branch with the changes, and how to run the network tests:

mkdir new_folder
git clone https://github.com/COVESA/dlt-daemon.git
git clone https://github.com/COVESA/vsomeip
cd vsomeip
git checkout cycle_event_repro
docker compose --project-directory zuul/network-tests build
docker compose --project-directory zuul/network-tests up

for docker to work it is important to have the folder with this contents:

new_folder
├── dlt-daemon
└── vsomeip

this way the cyclic test will run, check the result.
the test is located at https://github.com/COVESA/vsomeip/tree/cycle_event_repro/test/network_tests/cyclic_event_tests
See if you can reproduce it there.

if you have more changes to make this network test reproduce the problem, or a different reproduction you can provide, please do

[hjkcst]

@duartenfonseca

1. this only happens with 3.4.10? what about 3.5.5?
   I haven't tested version 3.5.5, but from the code perspective, there doesn't seem to be any difference in the part related to the cycle event.
2. i have tried with your correction, using ET_EVENT, and still it didn't happen.
   Do you mean that the following modifications were used, and did the "cycle event" still fail to stop? Is the cycle event still sending notifications?
   When testing a cycle event, calling application_impl::stop_offer_event is invalid. #666 #694 (comment)

[hjkcst]

@duartenfonseca
I think you can refer to the following code modifications:

[duartenfonseca]

@hjkcst i was able to do this, if the service has a cycle set on the configurations also. a note is that the server is not really notifying, because you don't see anything on the client side. The cycle function however is still running.

Can you update this PR to remove the current change, and add the last change that you mentioned, because that is the one that fixes the issue.

thanks

[hjkcst]

@duartenfonseca
I have push new change:

[duartenfonseca]

thanks, i will run it internally and see if it passes all the tests, will merge it

[duartenfonseca]

@hjkcst
now there are conflict issues, due to some new commits we added.
Another note, while testing this change, we found a lock ordering inversion:

Thread 1
event.cpp:161 ---> locks std::lock_guardstd::mutex its_lock(mutex_);
routing_manager_base.cpp:1255--> locks std::lock_guardstd::mutex its_lock(events_mutex_);

Thread 2
routing_manager_base.cpp:625 --> locks std::lock_guardstd::mutex its_lock(events_mutex_);
event.cpp:255 --> locks std::lock_guardstd::mutex its_lock(mutex_);

for this reason, i suggest replacing this change for the following:

void routing_manager_base::unregister_event(client_t _client, service_t _service, instance_t _instance,
            event_t _event, bool _is_provided) {

   (... no change until here ...)

    if (its_unrefed_event) {
       // adding here the unset, and not on the scope of  #events_mutex_
        if(_is_provided){
            its_unrefed_event->unset_payload(false);
            its_unrefed_event->set_provided(false);
        }

   (... no change after here ...)

        }
    }
}

[hjkcst]

@duartenfonseca
The conflict might be caused by the version I submitted being 3.4.10. Could you please incorporate this modification into the 3.4.10 branch first? Then synchronize it to the latest version of the code?

Do you mean like this change?

I think it should not be deleted here.

[duartenfonseca]

not its_event, rather its_unrefed_event. from my testing, with only the change I suggested, the notification cycle is stopped.
Can you give me an example where this does not work this way?

The conflict i am talking about is specifically regarding this pull request, on my local reproduction I did resolve the conflicts. I was just asking for you to do that change on your PR, so that it is done as you decide

[hjkcst]

@duartenfonseca
sorry.Please pay attention to the code in the first red box. I think this "unset" cannot be deleted. What do you think?

That is to say, I think the following code can be used as the latest revision. If we can reach an agreement, I will modify the code submitted this time and resubmit it.

[hjkcst]

@duartenfonseca

I have resubmitted the document according to the final revisions and resolved the conflict issues.
Could you please confirm if this modification will be synchronized in version 3.4.10?

thanks

[duartenfonseca]

The problem with that is that it will trigger the issue i mentioned here: #694 (comment)

we have to avoid calling unset_payload inside the scope of events_mutex_

[hjkcst]

@duartenfonseca
If this is not added, when a server registers a cycle event and then unregisters it, if ref is not equal to 0 (meaning that other processes are still using this event), the event will not be truly unregistered and the notification will not be stopped.
Although I haven't tested this scenario, I think it exists. What do you think?

[
else if (_is_provided) {
its_event->unset_payload(false);
its_event->set_provided(false);
}
]