search

suborbital / grav

Embedded decentralized message bus
Apache License 2.0
103 stars 8 forks source link

Memory leak in pod's messageFilter #86

Open jpcoenen opened 2 years ago

jpcoenen commented 2 years ago

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!

cohix commented 2 years ago

@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.

jpcoenen commented 2 years ago

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.

cohix commented 2 years ago

@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 😛

jpcoenen commented 2 years ago

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.