meteor-viewmodel
meteor-viewmodel copied to clipboard
dalgard
Binding twice after reconnect
Hi Kristian,
We are happily using your package for a while now. But since a while it seems that events are binded to HTML nodes twice after Meteor reconnects.
Have you seen this problem before?
Where is the code located which binds the event (within your package)? Maybe I can take a look ;)
More specifically: the click binding will get executed twice (or thrice, etc.) when the connection is lost.
I think the package expects the unbind function to be called when the elements are removed as a result of the data being unavailable, but we use GroundDB to ensure the data is available even when offline.
It seems the bind function is called again on reconnect, but the previous bind is not removed. (unbind is not called)
The strange this is that this seems to happen on some templates, but not others...
Note: I added a custom mousedown binding and the same occurs.
Note: when I console.log(this) within the viewmodel I can see the functions get executed after reconnection and a second nexus is added, but the first one is not removed.
Some pointers would be greatly appreciated :)
Thanks for the debugging effort. I'll look into it first thing, probably later today.
Basically the onReady gets executed, but the onInvalidate does not.
Is it possible that in
const computation = Tracker.currentComputation;
if (computation)
computation.onInvalidate(callback);
}
Tracker.currentComputation is undefined?
I'm pretty sure it is (null, not undefined), but now on to the why...
Could it be that because the datasource is GroundDB the computation is different?
But then why doesn't this happen on some of our other pages (where the datasource is also GroundDB, I verified it doesn't happen)...
I can verify that the unbind function is actually called on these elements after Meteor.reconnect() and ends up all the way at elem.removeEventListener with elem, type and listener properly defined (as far as I can tell).
So maybe it's not the unbinding going wrong, but the binding executing twice? Looking into that now.
Nope, bind gets called only once after reconnect, so I'm back to looking at the removeEventListener and why it's not working...
console.log(this.view)
this.view[ViewModel.nexusesKey].remove(this);
console.log(this.view)
Both view objects are identical and have identical contents (so no nexus is removed).
Is it possible that the listener has changed, which is why removeEventListener is not working and which would also explain by .remove(this) is not working (if it removes based on matching the object in an array)?
I also noticed that the vm-bind-id of all elements changes every time after calling meteor.reconnect() (this also happens on elements where we do not experience the multiple bindings problem).
If you set a custom attribute on one of those elements in dev tools, does the attribute disappear after the reconnect, meaning that the element is re-rendered?
I swapped jQuery for vanilla removeEventListener in 1.0.0 – would it be possible for you to downgrade to 0.9.4 and check if the problem is there, too? I suspect that it's not.
Let me know about the custom attributes. I definitely think you've found the correct part of the code to look at, but I'm having trouble reproducing the bug simply with Meteor.disconnect(), Meteor.reconnect(). The ids don't change in my page.
Something to do with computations is a good bet...
Yeah, the thing is it only happens on one set of elements in our code, all other sets are unaffected, but I don't see anything in those elements which is 'strange'. Would it help you to have a description of the element object?
Checking the custom attribute now (also re-added jQuery as a dependency and manually change the code back to jQuery in your package to test).
The IDs do change on all elements in our app, so it seems this might be caused by one 'bug' in combination with another...
wvanooijen92 is working on the same project... Don't know why he hasn't responded, he was also troubleshooting this.
I checked: when adding a custom attribute the attribute remains (indicating the element is not rerendered), but the vm-bind-id does change.
By the way: the vm-bind-id of ALL elements changes (not just those who depend on a GroundDB collection, also those who don't depend on a collection at all).
And damn, I forgot, but we are using GroundDB at ground:[email protected]
I just also tried switching back to jQuery (re-adding it as a dependency and using the .on and .off methods rather than the eventListener methods in plain Javascript but the problem persists.
I think the the problem of multiple bindings is VERY specific and only happens when elements are somehow 're-evaluated' by your package, causing the vm-bind-id to change. I am however yet to identify the difference between most elements in our project and these specific elements.
No reason for apology. It's definitely caused by some detail that has to do with the computation – what you call re-evaluation.
The hooks below should unbind the element, before it is bound anew.
this.onRefreshed(this.unbind);
this.onDestroyed(this.unbind);
this.onInvalidate(() => this.unbind(true));
Maybe one of these hooks doesn't run. Would you mind checking onRefreshed with a breakpoint on base.js#L102?
If the unbind function is called – please check which hook calls it by looking one level up in the call stack with a breakpoint on nexus.js#L237 – maybe the do_unbind parameter doesn't evaluate to true for some reason?
I can do you one better: the onInvalidate hook actually runs, the do_unbind value is true and it even ends up all the way at removeEventListener (with the (seemingly) correct element, binding type and listener, but it does not get removed. Op 15 mrt. 2016 10:23 a.m. schreef "Kristian Dalgård" < [email protected]>:
The hooks below (declared in the abstract Base class) should unbind the element, before it is bound anew. Maybe one of these hooks doesn't run. Would you mind checking that with a breakpoint on base.js#L102 https://github.com/dalgard/meteor-viewmodel/blob/master/packages/dalgard_viewmodel/lib/base.js#L102 ?
this.onRefreshed(this.unbind); this.onDestroyed(this.unbind); this.onInvalidate(() => this.unbind(true));If unbind is called (breakpoint on nexus.js#L237 https://github.com/dalgard/meteor-viewmodel/blob/master/packages/dalgard_viewmodel/lib/nexus.js#L237), maybe its do_unbind parameter doesn't evaluate to true for some reason.
— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub:
https://github.com/dalgard/meteor-viewmodel/issues/20#issuecomment-196737617
Maybe a new bind is triggered prematurely so that this.listener gets overwritten with a new listener, before unbind get a chance to unregister the old this.listener?
You might be able to debug that if you set a breakpoint where the listener is created (inside bind) and then right click the listener and select Store as global variable. Then you can compare that variable (temp1) with the listener that is unregistered in unbind.
(I have a tendency to edit my posts right after sending them, would it be possible for you to read directly on GitHub?)
I guess it's simple – a second call to bind should never occur unless unbind has run successfully.
Just the one I read on my phone (currently doing research for my thesis, so have to multitask a little).
I guess that makes sense (not calling a second bind on an element with the same type and same listener), do you still need me to check which method calls the unbind?
I don't think that's necessary, but it would be great if you could check whether bind is called the second time without unbind having been called successfully first – if you find the time.
It seems unbind is called before bind is called, re-checking at the point in time where the removeEventListener and addEventListener are actually called.
Ok, what happens for one specific element:
- bind is called
- unbind is called (the element, at this point, already has a new vm-bind-id)
- bind is called again
Afterwards: both binds are still functional (even though unbind is called and removeEventListener is reached, shame removeEventListener does not return true or false, but I think it is safe to say that one of the input variables for this function is not as it should be).
The guid of the listener in the 1st bind function is the same as the guid of the listener in the unbind function though...
I collected the Nexuses at 1 (bind called), 2 (unbind) and 3 (bind called again). (Right before addEventListener and removeEventListener.)
http://imgur.com/iEvmTW7
Is there anything you would like me to take a look at?
I wonder whether it would be possible for the listener to print its own guid when it fires? It would be interesting to see exactly what listeners are being called.
Ok, as I mentioned earlier I re-implemented jQuery in your latest version (just api.use('jquery') and added on and off instead of addEventListener and removeEventListener.
What I did now was remove the listener part of the function call ($(elem).off(type) instead of $(elem).off(type, listener) but this doesn't work either).
Trying $(elem).off() now (removing all event handlers).
Strange things are happening... When I call $(elem).off() (wihout type and listener) the bind event does not get fired a second time.
The result is obviously that the button no longer does anything.
Don't get me wrong: it's not that bind is called once, then bind is called and unbind removes both binds. It's that when unbind is called and .off() (without parameters) is used the second bind is never called...
Ok, using jQuery I was able to find out that the listeners actually do get removed (when I console.log jQuery._data(elem, "events") right after the .off call there are not click handlers attached.
I now think the Nexus is not properly removed and each nexus is re-bound when binding.
That must be due to some Blaze internals that aren't relevant to this issue.
I think that's much more likely. Glad we've solved the mystery around removeEventListener, at least.
Are there duplicate nexuses on the list if you inspect ViewModel.Nexus.find()?
Looking at the amout of Nexuses listed in the global list I don't think there are duplicates, but there are multiple Nexuses attached to a single element, that's for sure.
I just checked, there are not duplicates.
Somewhere there's a reference from the element to the nexus or vice versa that should be removed and garbage collected.
So. There are duplicate nexuses that involve the same binding-element pair, and each time Meteor reconnects, one more pops up. If you set a breakpoint inside bind, is it triggered twice, three times etc.? What about unbind, is it triggered the same number of times?
As far as I have been able to tell unbind is just triggered once, I'm looking at bind now.
Btw, you can filter the global nexus-list by passing in the element in question:
// $0 refers to the currently selected element in dev tools
ViewModel.Nexus.find($0);
How about this?
temp1 = some_superfluous nexus;
temp1.view.nexuses.indexOf(temp1) >= 0;
This indeed returns two Nexuses, so it seems I was wrong before (and you were right, I was looking through the entire list, rather than filtering them).
It is important that unbind isn't called twice when bind is. It must have something to do with some detail in onRefreshed, onDestroyed or onInvalidate. One of those hooks must fail to fire in your special situation.
I'm pretty sure unbind is 'working' (partially), because the .off() is being called (and the listeners are, if only very briefly, detached).
However I think that because the accompanying Nexus is not removed more than one listener is added when bind is called.
Right. So the view's onViewReady hook calls the bind function of both the old and the new nexus. That makes sense. That means unbind ought to somehow unregister bind from onViewReady (after which the nexus will be garbage collected instead of bound again).
The weird thing is that this problem only occurs sometimes. There must be something special going on with onViewReady that differs in your case.
In List.remove I do console.log(this, items) when it was called from unbind using this.view[ViewModel.nexusesKey].remove(this)
Then I get temp1 and temp2 and use temp1.indexOf(temp2) and get -1.
Because: temp1 contains two Nexuses with selector vm-bind-id 258 and 287, but temp2 contains one Nexus with selector vm-bind-id 128.
I'm fairly sure all these issues are related to the vm-bind-id being changed, for some reason.
The vm-bind-id attribute should change every time the view is refreshed – it is expected behavior. It's the only way the {{bind}} helper can be used in that special way that we use it.
I see, but because it is changed before/during unbind is running the object passed to remove does not match any of the objects in the list (I think).
Other places that would be interesting to inspect are view._callbacks and view._domrange.attachedCallbacks.
I've continuously profiled the usage examples for memory leaks and made sure there wasn't any, so I'm surprised that the library is bleeding nexuses in your case.
But I guess I haven't profiled any cases where a view is refreshed the way that it is in yours.
It would be helpful with a reduced test case that demonstrates this bug, but I know it isn't always a simple thing to do.
I don't have time to create a test-case currently (can only spend a couple of minutes at a time), but maybe tonight or tomorrow :)
I will keep working on trying to debug it though.
I've pinpointed the issue to the Tracker.afterFlush function in the onReady function; it fires twice.
Which makes perfect sense, since the onReady function also gets called twice, apparently.
I may not be able to look into it more until tomorrow, but I'm sure we will be able to fix this within a very short timeframe.
Great to hear, I've got a few more minutes today ;)
Continuing:
Which makes perfect sense again, since the element has had two nexuses attached to it. I'm not sure whether Tracker.afterFlush is the 'right' method to use here.
Anyhow I'm confident I got closer to the source of the issue, so I'm going to all changes and restart at onReady/Tracker.afterFlush.
Ok, I noticed something:
We have a list of 30 elements experiencing this issue, but sometimes some of the elements will result in a double bind whereas others will not... "Race condition-like" behavior.
It seems some things are just not completed in the expected order, but which things I don't know yet.
Also when I reconnect a second time some elements' viewmodel functions will fire twice whereas others will fire thrice.
Monkey patched the bug by adding
if (!view.afterFlushSet) {
Tracker.afterFlush(callback);
view.afterFlushSet = true;
}
On line 63 in base.js, but have yet to find the cause of the Nexuses not being removed.
Never mind, this causes other issues for the elements which were not problematic earlier...
Ok, what happens at the remove stage:
- A new Nexus is added and thus the vm-bind-id is changed.
List of Nexus.selectors: [vm-bind-id='250'] [vm-bind-id='282']
- The old Nexus should be removed, the remove function returns true (as in it has removed something), but instead the new Nexus is removed
Resulting in the new list of Nexus.selectors: [vm-bind-id='250']
- A few miliseconds later, something magic happens; a new Nexus is added to the list of nexuses: Resulting in the final list of Nexus.selectors: [vm-bind-id='250'] [vm-bind-id='334']
These two Nexuses together cause the double triggering of the viewmodel functions.
Ok, I further pinpointed the issue: unbinding works as expected in our scenario, it is the binding however which gets executed twice and messes things up (probably for the same reason Tracker.afterFlush gets called twice).
Would it help to know that these elements are rendered within a loop within a loop?
{{#each category}}
{{#each products}}
{{> productTemplate}}
{{/each}}
{{/each}}
Simplified representation (these are actually split over two templates).
I think it's an interesting perspective that we might be dealing with a race condition. Is it always the exact same elements that get the duplicate bindings?
Yes, but as I said it's a list of 30-something elements and there are times when elements 23 and 24 (for instance) are unaffected but the others are affected (which supports the view of 'race condition-like' circumstances).
If I understand you correctly, it is not always the exact same set of elements that gets affected?
It's 30-something elements which all make use of the same template, most of the times all of them are affected but on rare occasions some of them are unaffected.
So you are right in saying that not always all of the elements in the set are affected, but the phenomena is always limited to a subset of this specific set of elements.
My guess is that Meteor.reconnect() triggers a Tracker flush, which coincides with some or all elements being in a temporary state where the onReady or other event are supposed to happen before (or possibly after – I forget) an already scheduled flush.
The solution would be something along the lines of what you tried earlier. Hmm...
If you set a breakpoint on this line and look in the call stack, does that part of onReady ever get called directly by the Nexus constructor, or is it always inside a call to autorun (= irrelevant)?
Following the code down the rabbit hole:
For now I ended up at the bindHelper function of the viewModel which, apparently, gets called twice every time reconnect is called (for these elements).
However right after bindHelper is called for the second time the onDestroyed which resulted from the first bindHelper is called. The onDestroyed calls from the first bindHelper calls unbind on the element, but removing the Nexus returns false (I guess because the second bindHelper has already fired and changed the vm-bind-id of the element, causing a mismatch somewhere down the road).
I'm not sure why these elements are so rapidly re-rendered (possibly because GroundDB causes some computation to invalidate twice, rather than just once, on a reconnect), but I guess this could occur in other scenario's as well.
So I think we should make sure that when binding a helper any previously existing binding on the same element originating from the same Blaze helper should be properly removed.
The order of events:
bindHelper ONE viewmodel.js:512 newNexus nexus.js:114 ===== nexus.js:103 onDestroyed nexus.js:300 unbind nexus.js:313 false <== cannot remove nexus for some reason base.js:66 afterFlush nexus.js:255 bind <== we are binding here viewmodel.js:483 click: addProduct viewmodel.js:510 =====
bindHelper TWO viewmodel.js:512 newNexus nexus.js:114 ===== base.js:66 afterFlush nexus.js:255 bind <== we are ALSO binding here
Looking back at my previous reply I'm not sure whether unbind ==> false is the issue, it might rather be the fact that afterFlush is called twice resulting in two binds.
For sure, it could happen in other scenarios – thanks again for taking the time to debug this. I'll look into the reason why do_unbind evaluates to false.
The {{bind}} helper being triggered twice is expected behavior – dynamic attributes are re-run when anything about the element changes, which is the reason for many of the tricky maneuvers employed by the library.
Just to be sure, you are using the latest version 1.0.2, right?
I'm not really used to working with the stack directly, but the when I set a breakpoint on that line and look in the Chrome DevTools the second line says "nexus.js:111" which is this.onReady(this.bind) in the Nexus constructor.
The false I mention there is not the do_unbind, it is the result of this.view[ViewModel.nexusesKey].remove(this);
The nexuses list on the view is only for debugging and doesn't affect the bindings.
Do you think I should focus on why this.view[ViewModel.nexusesKey].remove(this) returns false or rather on why the bindHelper is called twice.
There are two differences between elements that don't have the erronous behavior and those who do. For elements that have the bug:
- the bindHelper is called twice after reconnect;
- the first unbind results in a false when calling this.view[ViewModel.nexusesKey].remove(this).
I see, if that is the case then Nexus.remove(this) a few lines earlier would probably also return false. I'll look into that.
It's weird if it returns false, but those two lines have no bearing on the bindings themselves, they only track what is currently on the page for debugging purposes.
The bind helper being called twice is expected behavior and only causes problems when the previous binding isn't unbound properly before each subsequent call.
Nexus.remove(this) also returns false.
But I see your point.
I do still think the afterFlush ==> bind being called twice is the culprit. Every time reconnect is called the previous binding is removed, but two new ones are added due to the afterFlush being triggered twice.
afterFlush should never trigger a bind if the previous bind has not yet been removed, I guess.
Actually I'm 100% sure this is the issue.
Even though Nexus.remove(this) returns false, before that the eventListener is removed so that should be fine (in terms of functionality, not in terms of memory leaks).
However because afterFlush is set twice (because it is bound in two different computations, one for the first triggering of bindHelper and another for the second) it is executed twice, but this shouldn't happen.
I'm still not sure whether afterFlush is the right 'tool for the job' (but I'm not sufficiently equipped to think of a better alternative).
Here's another idea: unbind accidentally runs before bind does, which would explain why Nexus.remove(this) returns false – it can't be removed from the global list, because it hasn't been added to it yet (which happens in bind).
The first unbind ought to run after the first bind, of course.
Don't pay too much attention to the multiple calls to bind/unbind, by the way – it's the only way that the library can be guaranteed to work in all situations and what makes it especially robust, in all modesty. Except in your case, of course ;)
I wonder whether view.onViewDestroyed sometimes runs before view.onViewReady? If the view is destroyed immediately, is the unbind callback run right away when registered? In that case, the value of view.isRendered and view.isDestroyed is important...
To muddle the image, list.add and list.remove will both trigger a Tracker flush.
Have we established exactly which hook is responsible for running unbind? Is it onRefreshed, onDestroyed, or onInvalidate?
I am using these console.logs:
this.onRefreshed(function() {
if (Meteor.checkElement(view)) {
console.log("onRefreshed")
}
that.unbind()
});
// Unbind element on view destroyed
this.onDestroyed(function() {
if (Meteor.checkElement(view)) {
console.log("onDestroyed")
}
that.unbind()
});
// Unbind element on computation invalidation
this.onInvalidate(function() {
if (Meteor.checkElement(view)) {
console.log("onDestroyed")
}
that.unbind(true)
});
In the Nexus constructor.