Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] bus: gc event at the end of a polling #31215

Open
wants to merge 1 commit into
base: 12.0
from

Conversation

Projects
None yet
6 participants
@Xavier-Do
Copy link
Contributor

Xavier-Do commented Feb 18, 2019

When making a poll, an event is registered on each channel. Once
a notification is sent on a channel, only the events of the
notified channel are removed. A pointer to the event is kept
in all other channel the user subscribes to until those channel
are notified.

Since a user usually has many mail.channels but only a few active ones,
a new event and a bunch of pointers are created at each poll and
never removed.

This fix simply remove the pointers to the current event at the
end of a poll.

@Xavier-Do Xavier-Do requested review from antonylesuisse and alexkuhn Feb 18, 2019

@robodoo robodoo added the seen 🙂 label Feb 18, 2019

@C3POdoo C3POdoo added the RD label Feb 18, 2019

@robodoo robodoo added the CI 🤖 label Feb 18, 2019

@Xavier-Do

This comment has been minimized.

Copy link
Contributor Author

Xavier-Do commented Feb 20, 2019

@tbe-odoo , @antonylesuisse wanted to try that on prod before merging, could you cherry pick this commit?

Note that I'm targeting 12 with this pr since it is a minor bug but we could add that in 10.0.

@odony

This comment has been minimized.

Copy link
Contributor

odony commented Feb 21, 2019

There is another "leak" with the channel keys stored in self.channels and never collected, even when there are no events in it anymore.

I haven't tested it, but it seems to me we would fix both leaks trivially if self.channels was a WeakKeyDictionary and its values were WeakSets instead of lists. We would not need to do any explicit GC, it would come from free automatically.

@Xavier-Do Xavier-Do force-pushed the odoo-dev:12.0-fix-bus-memory-leak-xdo branch Feb 21, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 21, 2019

@Xavier-Do

This comment has been minimized.

Copy link
Contributor Author

Xavier-Do commented Feb 27, 2019

@odony did we deploy this in prod for testing purpose?

@tbe-odoo

This comment has been minimized.

Copy link
Contributor

tbe-odoo commented Feb 28, 2019

@odony did we deploy this in prod for testing purpose?

Hello Xavier, sorry it was not deployed yet, but now it is :)

@Xavier-Do

This comment has been minimized.

Copy link
Contributor Author

Xavier-Do commented Feb 28, 2019

@tbe-odoo no problem, thanks!

@tbe-odoo

This comment has been minimized.

Copy link
Contributor

tbe-odoo commented Feb 28, 2019

@tbe-odoo no problem, thanks!

We tried for a bit and it seems like the chatter doesn't work anymore when applying this patch :/

[FIX] bus: gc event at the end of a polling
When making a poll, an event is registered on each channel. Once
a notification is sent on a channel, only the events of the
notified channel are removed. A pointer to the event is kept
in all other channel the user subscribes to until those channel
are notified.

Since a user usually has many mail.channels but only a few active ones,
a new event and a bunch of pointers are created at each poll and
never removed.

This fix simply remove the pointers to the current event at the
end of a poll.

@Xavier-Do Xavier-Do force-pushed the odoo-dev:12.0-fix-bus-memory-leak-xdo branch to 4279a4f Feb 28, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.