genkgo / ember-localforage-adapter

Offline usage for Ember Data, based on localstorage adapter, but now uses Mozilla's localforage as data source
Other
133 stars 26 forks source link

Fixes memory leak in utils/queue.js. #61

Closed iamolivinius closed 8 years ago

iamolivinius commented 8 years ago

Also fixes #60

frederikbosch commented 8 years ago

@iamolivinius One of the tests fail with this PR. Do you know why?

iamolivinius commented 8 years ago

Unfortunately there is no failing test on my local machine. Using node 5.7.0, phantomjs 2.1, and chrome 48

iamolivinius commented 8 years ago

I merged the latest master into my local copy today and rerun the tests in phantomjs 2.1.1 and chrome 53.0. In both environments all tests passed

------ RESULTS ------

default: PASS ember-release: PASS ember-beta: PASS ember-canary: PASS

frederikbosch commented 8 years ago

@iamolivinius I restarted the Travis build, but it fails. Do you know why?

iamolivinius commented 8 years ago

Not exactly yet but it's related to phantomjs. Afer I replaced phantomjs@2.1 with phantomjs@1.9 I got the same failures as in the travis environment.

iamolivinius commented 8 years ago

I run the tests in several saucelab environments and all passed. At this point in time I'd say the tests and the code under test is correct but the two year old versions of PhantomJS is somehow flawed :wink:

screenshot from 2016-09-27 21-19-34

iamolivinius commented 8 years ago

Wait, I cloned the wrong repo. :fearful: I'll rerun the tests.

iamolivinius commented 8 years ago

These are the actual results! It's always the same test that fails: "perform multiple changes in bulk => 2nd assertion". But I don't have any clue why the query doesn't return the previously saved result of the "Rambo" list.

screenshot from 2016-09-28 12-20-49

iamolivinius commented 8 years ago

Looks like the first build was OK and the rest had network issues.

First I rewrote the attach function to this:

import Ember from 'ember';
export default Ember.Object.extend({
  queue: [Ember.RSVP.resolve()],
  attach(callback) {
    const newPromise = new Ember.RSVP.Promise((resolve, reject) => {
      const previousPromise = this.queue.pop();
      previousPromise.then(() => {
        callback(resolve, reject);
      });
    });
    this.queue.push(newPromise);
    return newPromise;
  }
});

Then I realized that I spliced the wrong Promise from the queue array. I adapted the splice index et voilà! the tests pass continuously.