grav
grav copied to clipboard
suborbital
Memory leak in pod's messageFilter
Hey!
I have been digging into a memory leak with our product that uses Grav. During my search, I noticed ever-increasing memory consumption by github.com/suborbital/grav/grav.(*messageFilter).FilterUUID. Some further digging, has led to the following theory:
In pod.Send() p.FilterUUID(msg.UUID(), false) is called. In FilterUUID(), the UUID of the sent message is added to the UUIDMap map. This UUID seems to never be removed from the map again, leading to an increase in memory consumption every time pod.Send() is called.
I am happy to open an MR to address this, but my understanding of the internals of Grav are not super strong. So some guidance is certainly welcome!
@jpcoenen Yes you're absolutely right, that is a memory leak! I found this a while back but haven't had a chance to design a good solution. One idea I had was using a MsgBuffer (one of the other internal data structures), but haven't had time to sufficiently test. Happy to get on a call some time and pair on it, if you'd like. Ask Rick for my contact info or we can create a shared Slack channel.
Thanks for the quick reply, Connor!
If I understand your idea correctly, you propose to use a ring buffer for storing the messages that should be filtered? That would probably work in most cases, but what happens if the buffer fills up quickly? Could we run into a situation where a message gets evicted too soon?
If the idea is to make sure that a message does not end up back at the origin, would it be possible to add the origin to a message? So instead of keeping a list of messages that should be ignored, we drop a message if a pod sees that the origin stored in the message is the pod itself.
@jpcoenen that is an excellent idea, we'd just have to add a UUID to each pod! Much simpler. Happy to pair on it if you'd like.
Yes the ring buffer has several possible edge cases which is why I wasn't super motivated to implement it here 😛
Opened a quick MR for this: https://github.com/suborbital/grav/pull/87
Let me know if it aligns with what you had in mind. Feel free to propose any changes.