search

walmartlabs / thorax

Strengthening your Backbone
http://thoraxjs.org/
Other
1.32k stars 129 forks source link

Always cleanup bindToRoute #370

Closed kpdecker closed 10 years ago

kpdecker commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

candid82 commented 10 years ago

They should not be affecting each other though, right?

kpdecker commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

Updated to support the prior to route event case.

jhudson8 commented 10 years ago

+1 from me

candid82 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

@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 notifications@github.com 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/walmartlabs/thorax/pull/370#issuecomment-43325652 .

Sent from my mobile device

candid82 commented 10 years ago

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 notifications@github.comwrote:

@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 notifications@github.com 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/walmartlabs/thorax/pull/370#issuecomment-43354926 .

kpdecker commented 10 years ago

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 commented 10 years ago

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

candid82 commented 10 years ago

Tests failed, otherwise lgtm

kpdecker commented 10 years ago

Released in 3.0.0-alpha.6