Resolve "Japi_pushsrv_destroy is not cleaning ctx->push_service element" - [merged] #113

Closed fraunhofer-iis-bot closed 1 year ago

In GitLab by @vornkat-iis on Jun 30, 2023, 15:17

_Merges 43-japi_pushsrv_destroy-is-not-cleaning-ctx-pushservice-element -> dev

Closes #43

In GitLab by @vornkat-iis on Jul 2, 2023, 21:06

@bmi How do you run "japi_pushsrv_destroy at runtime"? japi_start_server is a blocking call, so usually push services cannot be modified while the server is running. Do you modify the context and then start it again? Or are you running it asynchron?

From inside japi_pushsrv_destroy I cannot access ctx which I would need to modify. What would you (as user and as SW architect) propose?

Document, that users need to clean up the linked list in ctx->push_services themselves if removing a push service. No modification to code.
Change the API to add ctx to the parameters of japi_pushsrv_destroy (could be a breaking change for some users) and clean up the list there
Add a wrapping function which handles the linked list and calls japi_pushsrv_destroy. Find a code proposal for that in 824dd8f207e3e038c019db8d855375a63dace011

In GitLab by @vornkat-iis on Jul 2, 2023, 21:36

added 1 commit

824dd8f2 - Add japi_pushsrv_deregister and tests

Compare with previous version

In GitLab by @vornkat-iis on Jul 2, 2023, 21:57

requested review from @Michael-M-Baron

In GitLab by @bmi on Jul 3, 2023, 08:46

i dont use it because it is not destroying the pointers. At the moment i have my own destroy function to destroy the pointers correct. I modify the context via a request function and my pointers are in a global context. I am not starting or Stoping anything. And i also not run asynchron.

to your points: 1: never do this your library is creating the pointer you must be sure to clean up everything.

2: yes this is the the only way you can handle it(at my point of view at the moment). And i think this is not a breaking change for somebody because if you not do it the code is broken and it will not run further so it is important to clean the list. (and nobody has report the problem bevor me, so i think nobody is using this at runtime)

I do not understand what you want to achieve

In GitLab by @vornkat-iis on Jul 3, 2023, 13:09

added 1 commit

295d04a8 - japi_pushsrv_destroy: remove from linked list

Compare with previous version

In GitLab by @vornkat-iis on Jul 3, 2023, 13:15

Assumption: It is save to change the signature of the function. If another user would access this function, they would have found the error sooner.

In GitLab by @vornkat-iis on Jul 4, 2023, 13:56

added 1 commit

95ec765f - Catch double free in japi_pushsrv_destroy

Compare with previous version

In GitLab by @vornkat-iis on Jul 4, 2023, 14:01

marked this merge request as ready

In GitLab by @vornkat-iis on Jul 4, 2023, 17:42

changed target branch from master to dev

In GitLab by @Michael-M-Baron on Jul 5, 2023, 10:52

added 8 commits

95ec765f...5b83a427 - 7 commits from branch dev
c1aefa93 - Merge branch 'dev' of git01.iis.fhg.de:ks-ip-lib/software/libjapi into...

Compare with previous version

In GitLab by @Michael-M-Baron on Jul 5, 2023, 10:57

I encountered this problem as well while developing the dynamic creating and removing of push services. See #47 for details.

The new approach with updating the ctx push service list in japi_pushsrv_destroy() right away looks good!

In GitLab by @Michael-M-Baron on Jul 5, 2023, 11:06

approved this merge request

In GitLab by @Michael-M-Baron on Jul 5, 2023, 11:06

mentioned in commit 0687def57673cd54407b686fd7870ea9c06afcc0

Fraunhofer-IIS / libjapi

Resolve "Japi_pushsrv_destroy is not cleaning ctx->push_service element" - [merged] #113