`<firebase-query>` sometimes return 2 sets of expected data on v2.2.0 #263

andrewspy · 2017-09-07T14:50:11Z

Description

<firebase-query> sometimes return 2 sets of expected data on v2.2.0. It was working fine with v2.1.x.

Expected outcome

data return a set of expected data.

Actual outcome

data return 2 sets of expected data

Live Demo

N/A

Steps to reproduce

You may need to use <firebase-query> more than once to reproduce.

Browsers Affected

Chrome

The text was updated successfully, but these errors were encountered:

tjmonsi · 2017-09-07T16:37:12Z

@andrewspy thanks for the headsup but can you give us on at least a working code we can copy or fork on?

Thanks

tjmonsi · 2017-09-07T16:38:06Z

Hmmmm... wait... I think I know this because I tried to replicate firebase-query on a project and tried to fix it. Lemme check the code

tjmonsi · 2017-09-07T17:18:37Z

I can't seem to replicate it even given this code:

    <firebase-query path="/test" data="{{data}}"></firebase-query>
    <firebase-query path="/test" data="{{data}}"></firebase-query>

    <template is="dom-repeat" items="[[data]]">
      [[item.value]]
    </template>

You can try giving us a sample, but it might be a race condition between firebaseOnValue and firebaseOnChildAdded

andrewspy · 2017-09-09T10:20:14Z

@tjmonsi Sorry for the late reply, it's a rather straight forward code as per your sample, but I am using the same <firebase-query> on 2 different views (as per PSK).

I'll try to reproduce it and provide a sample code later today.

tjmonsi · 2017-09-09T10:37:57Z

Yes please :) Thanks

andrewspy · 2017-09-09T18:03:14Z

@tjmonsi Here's the test case as promised, the problem occurs when you stamp the <my-query> twice.

<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
  <title>Polymer Element Test Case</title>
  <script src="./bower_components/webcomponentsjs/webcomponents-lite.js"></script>
  <link rel="import" href="./bower_components/polymer/polymer.html">
  <link rel="import" href="./bower_components/polymerfire/firebase-app.html">
  <link rel="import" href="./bower_components/polymerfire/firebase-query.html">

  <link rel="import" href="my-query.html">
</head>
<body>
  
  <h1>Duplicate firebase-query result</h1>
  <!-- Test case HTML goes here -->
  <firebase-app
    api-key="xxx"
    auth-domain="xxx"
    database-url="https://xxx.firebaseio.com"
    storage-bucket="xxx.appspot.com"
    messaging-sender-id="xxx">
  </firebase-app>

  <button onclick="addQuery()">Add Query</button>

  <script>
    function addQuery() {
      let newQuery = document.createElement("my-query");
      document.body.appendChild(newQuery);
    };
  </script>
</body>
</html>

and here's the code for my-query.html

<dom-module id="my-query">
  <template>
    <style>
      :host {
        display: block;
      }
    </style>

    <firebase-query log
      path="/test"
      data="{{data}}">
    </firebase-query>

    <p>
      [my-query]
      <template is="dom-repeat" items="[[data]]">
        [[item.value]]
      </template>
    </p>

  </template>
  <script>
    Polymer({
      is: 'my-query'
    });
  </script>
</dom-module>

Thanks for looking into the issue urgently!

tjmonsi · 2017-09-09T23:12:16Z

Thanks I'll check this up after my errands

jukbot · 2017-09-10T05:53:45Z

it seem like this is an issue from #170 😭

tjmonsi · 2017-09-10T15:18:27Z

Hi guys,

I have checked the firebase behavior and it is a race condition.

Somehow this happens:
If new-query is already existing in dom.

creation of new-query
creation of firebase-query
sets this.__initialLoadDone = false (so that on-child-added events will exit immediately)
sets listeners on on-value, on-child-added etc...
firebase-query asks data from firebase and saves it in memory
When retrieving, fires on-child-added one-by-one (as it should because data comes in "piece meal")
fires on-value at end to populate data.
sets this.__initialLoadDone = true (so that future on-child-added events work)

When you add new-query via dynamic event (like on-click)

creation of new-query
creation of firebase-query
sets this.__initialLoadDone = false (so that on-child-added events will exit immediately)
sets listeners on on-value, on-child-added etc...
firebase-query asks data from firebase
firebase sees data is already saved in memory, retrieves it whole
fires on-value first to populate data
sets this.__initialLoadDone = true (so that future on-child-added events work)
fires on-child-added one-by-one
with this.__initialLoadDone = true, retrieves data from memory and adds duplicates to array

See that before loading data to browser from firebase the event call was:

on-child-added n-times, on-value

When data is in browser

on-value, on-child-added n-times

Quick fix is to have a check on both on-value and on-child-added to check if the key is already existing in the data array, so duplicates will not be added.

But long term fix, I'm still thinking. Might be going to do a PR later this week (need to finish something for community and work).

@mbleigh or others might want to take a look at this

You can download a working repo to see the behavior: https://github.com/tjmonsi/polymerfire-fixers/tree/master/issue-263

andrewspy · 2017-09-10T19:20:53Z

@tjmonsi Just to share that the problem affect both polymer v1.9 and v2.0, so it may not be a polymer-v2 only issue.

tbeattysk · 2017-09-16T06:17:30Z

Not 100% sure if this is a good fix or not.
Seeing as the on-child-added calls are coming from the browser, they are being queued into the stack faster than on-value can complete. To push the initialLoadDone=True to the bottom of the stack we could use a setTimeout(()=>{}, 0) on line 346:

/* Now let child_added deal with subsequent changes */
          setTimeout(()=>{
            this.query.off('value', this.__onFirebaseValue, this);
            this.__initialLoadDone = true;
          }, 0); 
        },```

tjmonsi · 2017-09-17T03:56:13Z

I don't think that's the best fix, given that it is more hackish in a way that we are just "pushed" it to the last of the event stack but it is still unpredictable in terms of race conditions.

One thing I can think of this will fail is this:

on-value is called (for 3 children)
while executing on-value, in split second a new data was being added, putting on the event queue the on-value event and additional on-child-added event, making the event queue like

on-child-added (1st data)
on-child-added (2nd data)
on-child-added (3rd data)
on-child-added (new data)
on-value

we added setTimeout to turn off on-value and setting __initialLoadDone to true, adding another event in the event queue:
6. turn off on-value and setting __initialLoadDone = true

We call on-child-added 3 times without duplicating values because __initialLoadDone is still false (yey)

Running on-child-added of new data but not putting it because __initialLoadDone is not yet added

on-value is again called and add another event for turning off on-value (running number 4)
7. turn off on-value and setting __initialLoadDone = true (2nd time)

Running turn off on-value twice.

Yes, it might return 4 items, but that's a lot of function calls. What I think is best in this case is that,

on-value,
on-child-added (for the first existing data, but nothing is added)
on-child-added (new data)

tbeattysk · 2017-09-17T08:06:39Z

Ya felt that way too, it worked but would mess with the network load scenario.
Does it matter what order we query-on value and child? This seems to work too.
query.on('child_added', this.__onFirebaseChildAdded, this.__onError, this);
query.on('value', this.__onFirebaseValue, this.__onError, this);
query.on('child_removed', this.__onFirebaseChildRemoved, this.__onError, this);
query.on('child_changed', this.__onFirebaseChildChanged, this.__onError, this);
query.on('child_moved', this.__onFirebaseChildMoved, this.__onError, this);

tjmonsi · 2017-09-17T10:42:47Z

It is still unpredictable though. :( I'll create PR and let you guys see if it's ok

christophe-g · 2017-09-21T07:57:06Z

@tjmonsi - just curious (and also soon releasing an app to production ...), do you have any planned date for a new PR for this ?
thanks!

tjmonsi · 2017-09-21T08:47:19Z

@christophe-g working on it this weekend.

tjmonsi · 2017-09-21T16:27:00Z

@christophe-g I have time just now.
@andrewspy & @tbeattysk please check as well

You guys can check: #270

andrewspy · 2017-09-22T01:23:16Z

@tjmonsi The fix doesn't work with the test case I submitted above #263 (comment), as it always returns 2 sets of expected results now.

On my project, it also breaks some of my other <firebase-query>, with empty results.

tjmonsi · 2017-09-22T13:08:47Z

@andrewspy did you manage to use my branch? I'm going to check it again tonight

tjmonsi · 2017-09-22T13:19:15Z

I see it now. Going for a fix

tjmonsi · 2017-09-22T13:26:39Z

@andrewspy & @tbeattysk - added a fix. I think it was because I was still awake by 1am? But then again...

jukbot mentioned this issue Sep 10, 2017

firebase-query duplicating values #244

Open

1 task

tjmonsi mentioned this issue Sep 21, 2017

fix firebase query returns 2 sets for #263 #270

Merged

tjmonsi closed this as completed in 645371a Sep 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`<firebase-query>` sometimes return 2 sets of expected data on v2.2.0 #263

`<firebase-query>` sometimes return 2 sets of expected data on v2.2.0 #263

andrewspy commented Sep 7, 2017

tjmonsi commented Sep 7, 2017

tjmonsi commented Sep 7, 2017

tjmonsi commented Sep 7, 2017

andrewspy commented Sep 9, 2017

tjmonsi commented Sep 9, 2017

andrewspy commented Sep 9, 2017

tjmonsi commented Sep 9, 2017

jukbot commented Sep 10, 2017 •

edited

Loading

tjmonsi commented Sep 10, 2017

andrewspy commented Sep 10, 2017

tbeattysk commented Sep 16, 2017 •

edited

Loading

tjmonsi commented Sep 17, 2017

tbeattysk commented Sep 17, 2017

tjmonsi commented Sep 17, 2017

christophe-g commented Sep 21, 2017

tjmonsi commented Sep 21, 2017

tjmonsi commented Sep 21, 2017

andrewspy commented Sep 22, 2017

tjmonsi commented Sep 22, 2017

tjmonsi commented Sep 22, 2017

tjmonsi commented Sep 22, 2017

<firebase-query> sometimes return 2 sets of expected data on v2.2.0 #263

<firebase-query> sometimes return 2 sets of expected data on v2.2.0 #263

Comments

andrewspy commented Sep 7, 2017

Description

Expected outcome

Actual outcome

Live Demo

Steps to reproduce

Browsers Affected

tjmonsi commented Sep 7, 2017

tjmonsi commented Sep 7, 2017

tjmonsi commented Sep 7, 2017

andrewspy commented Sep 9, 2017

tjmonsi commented Sep 9, 2017

andrewspy commented Sep 9, 2017

tjmonsi commented Sep 9, 2017

jukbot commented Sep 10, 2017 • edited Loading

tjmonsi commented Sep 10, 2017

andrewspy commented Sep 10, 2017

tbeattysk commented Sep 16, 2017 • edited Loading

tjmonsi commented Sep 17, 2017

tbeattysk commented Sep 17, 2017

tjmonsi commented Sep 17, 2017

christophe-g commented Sep 21, 2017

tjmonsi commented Sep 21, 2017

tjmonsi commented Sep 21, 2017

andrewspy commented Sep 22, 2017

tjmonsi commented Sep 22, 2017

tjmonsi commented Sep 22, 2017

tjmonsi commented Sep 22, 2017

`<firebase-query>` sometimes return 2 sets of expected data on v2.2.0 #263

`<firebase-query>` sometimes return 2 sets of expected data on v2.2.0 #263

jukbot commented Sep 10, 2017 •

edited

Loading

tbeattysk commented Sep 16, 2017 •

edited

Loading