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

[Browser Client] Minor memory leak: ".mail_source" event resubscriptions #1911

Closed
jvilk opened this issue Aug 17, 2017 · 4 comments
Closed

[Browser Client] Minor memory leak: ".mail_source" event resubscriptions #1911

jvilk opened this issue Aug 17, 2017 · 4 comments

Comments

@jvilk
Copy link
Contributor

@jvilk jvilk commented Aug 17, 2017

Problem

Every time the main view is redrawn, Mailpile adds a listener for ".mail_source" events in the EventLog. Mailpile never removes these listeners, so this is a memory leak.

Cause

Mailpile evals the following code every time the main view is redrawn:

$(document).ready(function() {
  Mailpile.Search.init();
  Mailpile.Tags.init();
});

Mailpile.Search.init(); contains the following line of code that subscribes to the ".mail_source" event:

  EventLog.subscribe(".mail_source", function(ev) {
    // bre: re-enabling this just for fun and to test the event subscription
    //      code. This is broken in that it fails for non-English languages.
    if (ev.message.indexOf("Rescanning:") != -1) {
      $("#logo-bluemail").fadeOut(2000);
      $("#logo-redmail").hide(2000);
      $("#logo-greenmail").hide(3000);
      $("#logo-bluemail").fadeIn(2000);
      $("#logo-greenmail").fadeIn(4000);
      $("#logo-redmail").fadeIn(6000);
    }
    $('.status-in-title').attr('title', ev.data.name + ': ' + ev.message);
  });

Over time, EventLog.eventbindings fills with many subscribers from this call site.

Suggested Fix

Add code that checks for an existing subscription, and avoids resubscribing if one exists.

@BjarniRunar
Copy link
Member

@BjarniRunar BjarniRunar commented Aug 19, 2017

Thank you for filing this. I agree with your analysis and I think you've done a great job describing the problem. 👍

To expand on how to fix; in shared-data/default-theme/html/jsapi/global/eventlog.js, there is a subscribe method which needs to be changed:

  1. Add a third free-form ID string to subscriptions (a new argument)
  2. Instead of a list of callbacks, use a dictionary/object keyed on the ID string.
  3. If no ID is provided, generate a random one

The functions that work with the subscription list will need to be modified to iterate over the new object values, and any calls to EventLog.subscribe should be updated to add an ID (point 3. above will allow any old code that gets overlooked to continue working, albeit with the memory leak).

This design will allow prevention of duplicate registrations, while still allowing multiple parties to subscribe to the same events.

@suryajoshi007
Copy link

@suryajoshi007 suryajoshi007 commented Sep 9, 2017

Would love to work on this. Can i get started?

@BjarniRunar
Copy link
Member

@BjarniRunar BjarniRunar commented Sep 14, 2017

@suryajoshi007 actually, there is already a pull request open. So maybe find something else? :)

@BjarniRunar
Copy link
Member

@BjarniRunar BjarniRunar commented Mar 8, 2018

This got fixed! Thanks everyone!

@BjarniRunar BjarniRunar closed this Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants