Bug/3162 registration destroys compound values #28
Conversation
…tion_destroys_compound_values
| @@ -52,6 +52,79 @@ using namespace orion; | |||
|
|
|||
|
|
|||
|
|
|||
| typedef struct CompoundSave | |||
There was a problem hiding this comment.
If at the end we go with the workaround implemented by this PR, an explanation of the workaround should be included in a comment in the code in order to guide for a definitive solution. The text in the PR body is a good starting point for that.
Issue telefonicaid#3162 would remain open until the definitive solution comes.
| std::string state; | ||
| } CompoundSave; | ||
|
|
||
| static CompoundSave compoundSaveV[100]; |
There was a problem hiding this comment.
100 should be unhardwired to a #define (maybe in this same file, no need to include it in global.h in the case of a workaround)
| csP->aParentP = aParentP; | ||
| csP->state = "used"; | ||
|
|
||
| ++compoundSaveIx; |
There was a problem hiding this comment.
What about if this index overpass 100 ?
| { | ||
| if (aP->compoundValueP == compoundSaveV[ix].compoundP) | ||
| { | ||
| if (compoundSaveV[ix].state == "used") |
There was a problem hiding this comment.
Style:
if ()
{
}
(Pls review the PR, I have seen more of this in other places)
| csP->name = aP->name; | ||
| csP->compoundP = aP->compoundValueP; | ||
| csP->aParentP = aParentP; | ||
| csP->state = "used"; |
There was a problem hiding this comment.
Maybe an enum (instead of plain strings) should be used for states.
| @@ -569,6 +569,8 @@ static void requestCompleted | |||
| std::string spath = (ciP->servicePathV.size() > 0)? ciP->servicePathV[0] : ""; | |||
| struct timespec reqEndTime; | |||
|
|
|||
| // compoundSaveReport(); | |||
There was a problem hiding this comment.
If this comment is preserved at the end (seems to be a good idea), a FIXME mark citing telefonicaid#3162 should be included in the line above it.
| @@ -0,0 +1,368 @@ | |||
|
|
|||
There was a problem hiding this comment.
Blank line could be removed.
| @@ -23,15 +23,13 @@ | |||
| # | |||
There was a problem hiding this comment.
I understand this test was already include as "issue bug demostration" and the present PR is enabling it (and fixing some parts, I guess)
There was a problem hiding this comment.
In fact... appart from "format changes" (e.g. echo, content-length adjust, JSON pretty print, etc.) is there any real change in the PR?
There was a problem hiding this comment.
The real change is that instead of just copying the compound pointer in ContextAttribute::ContextAttribute(ContextAttribute* caP, bool useDefaultType),
the compound is cloned.
There was a problem hiding this comment.
This change gave leaks that were ... really hard to understand.
My guess: timing.
To understand the leaks I saved a pointer of all compounds cloned right there in "the real change", and on free I remove those pointers, to finally free those that were still in the vector.
[ I just remembered this ] Imagine my surprise when finding that there were no compounds left to free.
However, if I remove the saving-compound-pointers-to-vector, then I get memory leaks.
So, the vector is "unneeded code" that is essential due to the timing it changes. Without it, we get memory leaks.
And now you know why I don't like this fix ...
It works, but more time for investigation is needed. Doesn't have to be right now. Especially not as this fix is needed.
There was a problem hiding this comment.
Ups... I didn't make myself clear enough. I mean, not the changes in the PR in general, but the changes of the .test in the PR, i.e.:
In fact... appart from "format changes" (e.g. echo, content-length adjust, JSON pretty print, etc.) is there any real change in this .test?
There was a problem hiding this comment.
(The explanation about the PR itself is good... but I'll move to the general comment in the PR to continue discussion)
| # | ||
| # 01. Create entity E1 without attributes | ||
| # 02. Append new geo:box attribute (attr3) -> OK | ||
| # |
There was a problem hiding this comment.
A step 03 with GET to check that the attributes has been really appended should be included.
There was a problem hiding this comment.
Need to take a look at this test to understand what has happened.
The way it is ... makes no sense whatsoever.
The idea behind step 2 is to actually find the entity (entities without attr that have a registration weren't found).
A recent PR of mine fixes this.
But, I found that bug myself implementing the fix of this PR.
I don't care about the new entity after Append. Only that the append works (i.e., the entity was found)
There was a problem hiding this comment.
Si, I looked into this a little ...
This test I used simply to provoke the bug fixed in telefonicaid#3193.
its current contents is what remains after tests, with the registration removed.
The bug is fixed, and has its own test. This test is no longer needed and will be removed.
| @@ -0,0 +1,82 @@ | |||
| # Copyright 2016 Telefonica Investigacion y Desarrollo, S.A.U | |||
There was a problem hiding this comment.
This test is under directory 3162_context_providers_legacy_non_primitive/ but it seem is has nothing to do with CP... maybe should be moved to another directory? Maybe is already covered (maybe grep for "geo:box" could help)?
There was a problem hiding this comment.
The test is called registration_without_provider.test ...
But, what happened to the registration? It's gone.
I guess I was experimenting and forgot to revert it.
Once t he registration is back, you will see the connection to CPs.
|
@kzangeli said:
|
So your theory is that the saving-compound-pointers-to-vector introduce some time delay that avoid the leaks? |
|
That is me theory right now, yes.
Need more time to develop a better theory
…On Fri, Jun 8, 2018 at 10:40 AM, Fermín Galán Márquez < ***@***.***> wrote:
However, if I remove the saving-compound-pointers-to-vector, then I get
memory leaks.
So, the vector is "unneeded code" that is essential due to the timing it
changes. Without it, we get memory leaks.
So your theory is that the saving-compound-pointers-to-vector introduce
some time delay that avoid the leaks?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFF1DKjL8Q2ALnyTExY5Yn2zoGJRtmHmks5t6jiQgaJpZM4T41EX>
.
|
|
In that case, why don't just replace the saving-compound-pointers-to-vector with a sleep() statement? It would require some tunning to find the right value, but it would be much more simpler and easy to adjust (as a "gauge" if the delay needs to be adjusted later). |
|
Of course, if the vector is really not needed finally, there is no reason
to keep it.
This is still very cryptic. I don't understand what is happening.
Will try with a sleep and it that removes the leak then it would be a much
better workaround that the current one :)
…On Fri, Jun 8, 2018 at 11:02 AM, Fermín Galán Márquez < ***@***.***> wrote:
In that case, why don't just replace the saving-compound-pointers-to-vector
with a sleep() statement? It would require some tunning to find the right
value, but it would be much more simpler and easy to adjust (as a "gauge"
if the delay needs to be adjusted later).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFF1DC2dAz1-CDH2i32os2UfEbxvRjlOks5t6j2XgaJpZM4T41EX>
.
|
|
@kzangeli @fgalan - I've just built this branch locally, and I can confirm that the changes do indeed work and will fix the issue. I'm surprised that so few users are using compound entities within their context data and that no-one else has come across the issue. For example, if I am a GPS unit, I should be storing by data a GeoJSON point right? Therefore GPS units should be producing compound values. |
Not necessarily... GeoJSON point can be represented using geo:point attribute type with value e.g. "40.23,-3.23" which is a string. |
|
Thanks for the feedback @jason-fox ! :) |
Fixed issue telefonicaid#3162 ... I think ...
The only real change is that in
ContextAttribute::ContextAttribute(ContextAttribute* caP, bool useDefaultType)the compound is cloned and not "set to point to the same pointer".I encountered some really important problems with leaks and when implementing help code to understand the leak, the leak ... disappeared ... :(
I have no explanation for this except that ... the current implementation works, with the vector of cloned compounds that later isn't used for anything at all, it just changes the timing.
So, the leak was a timing problem, not new to us ...
However, if the problem comes back, this new vector will be useful.
In lack of a real fix, this "workaround" solves the problem, at least temporarily ...