[qfix] Fix panic on metadata chain elements double Close #1036
Conversation
Bolodya1997
force-pushed
the
qfix/metadata-double-close
branch
from
1b40c65
to
bb6ed62
Compare
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Bolodya1997
force-pushed
the
qfix/metadata-double-close
branch
from
bb6ed62
to
ce024da
Compare
| if err != nil { | ||
| del(ctx, connID, &m.Map) | ||
| m.Map.Delete(connID) |
There was a problem hiding this comment.
How is the Close call going to access the metadata it needs to clean up resources? Before it was guaranteed access to that data by del()... now it will not have it at all and we should expect to leak all resources...
There was a problem hiding this comment.
Close is currently making:
next.Server(ctx).Close(store(…), conn)
So it has access to metadata.
Actually the problem before was Close making:
next.Server(ctx).Close(del(…), conn)
And so on double Close there were no metadata.
| md, _ := mdMap.LoadAndDelete(id) | ||
| return context.WithValue(parent, metaDataKey{}, md) | ||
| } | ||
| return parent |
There was a problem hiding this comment.
Its seems to me the better fix here would be:
| return parent | |
| return context.WithValue(parent,metaDataKey{},&metaData{}) |
There was a problem hiding this comment.
I don't think you quite understood my suggestion... I believe that just this change will solve your panic issue, no need for other changes. Small, simple, targeted.
There was a problem hiding this comment.
Oh wait... I read to quickly... you did fix this :)
There was a problem hiding this comment.
So actually this fix leads to the same error, because on L40 we are getting md == nil and so on L41 we just storing it into the context.
But this direction looks better, so:
if _, ok := parent.Value(metaDataKey{}).(*metaData); !ok {
if md, ok := mdMap.LoadAndDelete(id); ok {
return context.WithValue(parent, metaDataKey{}, md)
}
return context.WithValue(parent, metaDataKey{}, new(metaData))
}
return parentSigned-off-by: Vladimir Popov <vladimir.popov@xored.com>
Description
Fixes
metadata.Map(ctx)panic on double Close.Issue link
Related to kind integration tests failure - https://github.com/networkservicemesh/integration-k8s-kind/runs/3136678807?check_suite_focus=true.
How Has This Been Tested?
Types of changes