Always cleanup bindToRoute

[kpdecker]

If someone is explicitly calling Backbone.history.loadUrl on the same
route then the bindToRoute logic will leak as the event handler is never
cleaned.

Since this is something that presumably is relatively rare and backbone
itself does some cleanup and routing events in response to this,
restructuring the bindToRoute handle so that it will terminate the bound
events in response to any route event.

/cc @eastridge @ryan-roemer @candid

[candid82]

I don't quite understand the problem here. Could you please elaborate on why calling loadUrl() leads to a leak? What I am seeing in the code is that route event with the same route will be ignored as far as bindToRoute logic is concerned.
The deal with bindToRoute is: you pass it a callback and a failback. It returns another function (result). When you call the result, it will execute the original callback only if the route has not changed. If it the route did change before you called the result, failback will be executed instead (on route change). If you no longer want to call the result but also don't want the failback to execute on route change, you have to call result.cancel().
Part of the contract here is that callback will only be suppressed (and failback called instead) if the route actually changed, receiving route event with the same route is not enough. Changing this may break depending projects in subtle ways.

[kpdecker]

So in situations like SSJS where we may commonly reuse routes, which leads to leaks/retention due to the route event bind. The memory will be released once a different route string comes in, but at least under the current distribution we have this is few and far between, leading to GC strain, etc.

This is a breaking change but we are still in prerelease version on the 3.x line so we're fine from that aspect and it only impacts people who use loadUrl directly so we can easily document the risk.

I think the bigger question is if a route has changed when the route comes in but it is on the same path. I reasoned through this with @jhudson8 and came to the conclusion that it was since it's (1) a very explicit call by the client and (2) it means that the controller has executed and the objects that the bind to route was tied to are likely pertinent.

[candid82]

So separate requests may hit the same route on the server side since we are reusing pages / VMs (or whatever you want to call them)?

[kpdecker]

Yeah. We can run into MANY / requests in a row.

[candid82]

They should not be affecting each other though, right?

[kpdecker]

If this is merged then they will be isolated. If this is not merged then there is a chance (unlikely with the pending queue...) that /home request 1 could impact /home request 2.

[kpdecker]

I'm almost certain that I figured out why this code was in here in the first place, if these are run in the handler directly then they will cancel as the route event occurs after the handler execs. I still stand by the conclusion that explicit loadUrl calls should cancel bindToRoute instances but I need to revisit to make sure that the initial route event doesn't blow up.

[kpdecker]

Updated to support the prior to route event case.

[jhudson8]

+1 from me

[candid82]

This makes the logic even more complicated. I just spend 20 minutes trying to understand what problem it's solving, and I am sure if I come back to this code in a week I will have to spend another 20 minutes. I still don't understand why we are making breaking changes to make things work with separate requests on the server side when they are supposed to be completely isolated. Part of the problem I guess is that I am lacking understanding of how reuse works on the server side. If one request can trigger a route event in another request, is not it a problem?

[jhudson8]

It's only a problem if you trigger a route that you are already on because the bindToRoute handler will not execute the callback which will stick around

[kpdecker]

@candid I'll add docs to the code. It certainly is not obvious what this is
doing if you didn't just write it.

We all agree that we don't want code to execute against a different route.
Can we agree that a route is different at any point that the routes handler
executes?

On Friday, May 16, 2014, Joe Hudson [email protected] wrote:

It's only a problem if you trigger a route that you are already on because
the bindToRoute handler will not execute the callback which will stick
around

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/370#issuecomment-43325652
.

Sent from my mobile device

[candid82]

But it's not always different :) Regardless, it's not the actual change
that concerns me more at this point, it's the motivation behind it. If
another requests reuses the same page and triggers route event, previous
request should not be affected because it should have been fully processed
by that time and no handlers should be lingering around. This should be
enforced by the environment, maybe by calling Backbone.history.off('route')
after the page emitted. Am I missing something?

-Roman

On Fri, May 16, 2014 at 10:01 AM, Kevin Decker [email protected]:

@candid I'll add docs to the code. It certainly is not obvious what this
is
doing if you didn't just write it.

We all agree that we don't want code to execute against a different route.
Can we agree that a route is different at any point that the routes
handler
executes?

On Friday, May 16, 2014, Joe Hudson [email protected] wrote:

It's only a problem if you trigger a route that you are already on
because
the bindToRoute handler will not execute the callback which will stick
around

—
Reply to this email directly or view it on GitHub<
https://github.com/walmartlabs/thorax/pull/370#issuecomment-43325652>
.

Sent from my mobile device

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/370#issuecomment-43354926
.

[kpdecker]

Blanket removing events is a very bad thing. This was the reason for one of the leaks we had in the hapi proxy. You should never remove events that you don't explicitly own, unless you KNOW that you explicitly own a particular object. Backbone.history is not such a case as it's both globally accessible and anyone can register whatever they please.

Since we aren't making progress on this I'm going to make the call that a bindToRoute is tied to a given execution of a given controller handler. If it is called twice, it is considered a different route.

I've updated the patch to include further comments as to what the event handler is doing to make this clearer in the future as well as tests to improve coverage.

[kpdecker]

Updated with changes that also help protect against issues with bindToRoute prior to start (#311)

[candid82]

Tests failed, otherwise lgtm

[kpdecker]

Released in 3.0.0-alpha.6