RSDK-12383 Stop leaking resource.Resources with resLoggers in module code #5398
Conversation
viambot
added
the
safe to test
module/module.go
Outdated
| handlers HandlerMap | ||
| collections map[resource.API]resource.APIResourceCollection[resource.Resource] | ||
| resLoggers map[resource.Resource]logging.Logger | ||
| resLoggers map[resource.Name]logging.Logger |
There was a problem hiding this comment.
I see why you did this, but this requires a different kind of analysis.
There was a problem hiding this comment.
different kind of analysis
Do you mean proving (or at least being somewhat sure) that we have a bijection of resource names to loggers in a single Golang module? Whereas mapping resource.Resource:logging.Logger is more "obviously" correct in that we don't have to ask the question "do names always uniquely and correctly identify resources in all usages of resLoggers"?
And, if so, are you saying that to suggest I don't try to prove that since it's not worth the effort/commitment to that invariant?
module/module.go
Outdated
|
|
||
| delete(m.streamSourceByName, res.Name()) | ||
| delete(m.activeResourceStreams, res.Name()) | ||
| delete(m.resLoggers, res.Name()) |
There was a problem hiding this comment.
Is it guaranteed that all requests for a resource are drained before RemoveResource can be processed?
There was a problem hiding this comment.
That's a good question; I believe the answer is no. I just tried RemoveResourceing a Golang modular component in the middle of its DoCommand invocation. We certainly don't have any logic now that waits for that DoCommand to finish before completing RemoveResource.
Are you asking that question because you're concerned that deleteing the logger from the resLoggers map could cause the logger to be garbage collected while a request is still using it?
benjirewis
force-pushed
the
missing-resLoggers-delete
branch
from
2b92571 to
178de19
Compare
viambot
added
safe to test
benjirewis
left a comment
benjirewis
left a comment
There was a problem hiding this comment.
Thanks for the review! I've responded to your questions with, unfortunately, more questions 😆 .
module/module.go
Outdated
| handlers HandlerMap | ||
| collections map[resource.API]resource.APIResourceCollection[resource.Resource] | ||
| resLoggers map[resource.Resource]logging.Logger | ||
| resLoggers map[resource.Name]logging.Logger |
There was a problem hiding this comment.
different kind of analysis
Do you mean proving (or at least being somewhat sure) that we have a bijection of resource names to loggers in a single Golang module? Whereas mapping resource.Resource:logging.Logger is more "obviously" correct in that we don't have to ask the question "do names always uniquely and correctly identify resources in all usages of resLoggers"?
And, if so, are you saying that to suggest I don't try to prove that since it's not worth the effort/commitment to that invariant?
module/module.go
Outdated
|
|
||
| delete(m.streamSourceByName, res.Name()) | ||
| delete(m.activeResourceStreams, res.Name()) | ||
| delete(m.resLoggers, res.Name()) |
There was a problem hiding this comment.
That's a good question; I believe the answer is no. I just tried RemoveResourceing a Golang modular component in the middle of its DoCommand invocation. We certainly don't have any logic now that waits for that DoCommand to finish before completing RemoveResource.
Are you asking that question because you're concerned that deleteing the logger from the resLoggers map could cause the logger to be garbage collected while a request is still using it?
benjirewis
force-pushed
the
missing-resLoggers-delete
branch
from
178de19 to
a0dfbe4
Compare
viambot
added
safe to test
benjirewis
left a comment
benjirewis
left a comment
There was a problem hiding this comment.
@dgottlieb apologies for delay on this PR. I retained the original map typing and just added some missing deletes on resource rebuild and removal. Let me know what you think.
|
Sorry @benjirewis , I never responded to those questions. Or maybe we talked offline, can't recall! I actually have a PR that might be addressing this (need to look closer again). But this PR was why I noticed in the first place. I'll know better later this week. |
|
No worries! Gotcha, will hold off on this PR until EoW, then. Let me know if you need more eyes on your PR for RSDK-12391. |
3 participants
Changes type of
resLoggersin Golang module library to bemap[resource.Name]logging.Loggerinstead ofmap[resource.Resource]logging.Loggerto avoid keying onresource.Resourceand accidentally leakingresource.Resourceobjects when we forget todelete. Adds a missingdeletefor the map inRemoveResource. Adds a test to ensure thatresLoggerslooks as expected after adds, reconfigures, and removes.