fix(ssr): memory leak in poll method

[ronald-d-rogers]

Fix #2606.

Unlike #2867, this fix does not depend on the following being merged first: vuejs/vue#9479

Removed poll method, which was causing a memory leak when a Vue app is created per request, which is typical in SSR.

The leak happens when:

1. The router-view is conditioned to appear upon a variable that never becomes true.

<div id="app">
  <div v-if="condition">
    <router-view />
  </div>
</div>

2. A beforeRouteEnter guard's next is passed a cb:

export default {
  beforeRouteEnter(to, from, next) {
    // Poll happens only if you pass in a function to next(...)
    next(vm => {} /* cb */)
  }
}

Instead of polling we try to call the cb in History#confirmTransition, otherwise add the cb to RouteRecord#enteredCbs, and call it instead upon route registration (see components/view.js).

I don't know whether or not it is ok to add enteredCbs to RouteRecord. I don't know whether or not we are ok with mutating the route record inside of History#confirmTransition. I also don't really know if this will cause problems elsewhere outside of running npm test.

The apps my team builds here in Bloomberg are thoroughly E2E tested. If you don't see any problems with this approach I can try to get our tests run against this fix version, if that helps any.

[ronald-d-rogers]

Ah I see, the E2E tests didn't run locally for some reason. Let me see if I can fix these errors.

[posva]

Could you rebase against dev? There were a few things in between

[ronald-d-rogers]

@posva This still does not work. I created an E2E test for the out-in transition problem (#750), and the test fails. I will continue to play with it, but this in its current state should not be merged.

[ronald-d-rogers]

I believe I have a solid fix now. Will clean up and commit new E2E test hopefully tomorrow.

[ronald-d-rogers]

@posva There are some edge cases I'm a little worried about, such as the possibility of the beforeRouteEnter next callbacks getting called before transitionTo's onComplete hook gets called. handleRouteEntered is also called upon instance registration in the component's init hook now so it seems possible. I also will still likely run this fix against the memory leaking builds I had in my apps a few months ago. You can wait for that if you want, but otherwise I feel somewhat confident about this fix.

[ronald-d-rogers]

I finally got around to testing a past build that had the memory leaks against this fix, here are the results.

E2E Tests

All E2E tests pass for both builds

With memory leak:

Without memory leak:

Memory Leak Test

With memory leak

We do not build with the fixed version of vue-router (no arrow)

We siege a route with the condition detailed above with 1000 hits

CPU profile shows a busy CPU filled with polls

An examination of New Relic shows that the memory stays at ~600MB for 40 minutes after the siege. It also shows 90% CPU utilization and an event loop with 10k ticks per minute even though the website is not being accessed at all.

Without memory leak

We build with the fixed version of vue-router (see the arrow pointing outwards)

We siege the same route with 1000 hits

CPU profile shows that it is doing nothing

New Relic shows that the memory does not accumulate above 100MB, a low GC pause frequency and a serene event loop after sieging.

I can play around with the fix some more to see if there are any edge cases to deal with, let me know if you are waiting on this and I will get to it sooner rather than later.

[posva]

@ronald-d-rogers oh wow, thanks a lot for such a detailed report. If you think all is ready, I will give it a look this week see if we can get it merged and released in a patch

[ronald-d-rogers]

@posva I still want to feel around the edges to see if there are any issues but I don't think there will be and even if there are it will only affect this particular use case (next callbacks in beforeRouteEnter hooks), so I think I'm ok with it. I will try to test that one edge case that I am slightly worried about soon anyways, probably this weekend.

[ronald-d-rogers]

@posva I put console log statements in the beginning of confirmTransition, onComplete, handleRouteEntered, ran all E2E tests, and saved all the browser's console statements to a file. They consistently run in order in all tests with the exception of lazy-loading-before-mount, where Homes next callbacks (handleRouteEntereds) are called after Foo's confirmTransition starts. Given the way the test is written I think that is expected. That's the only thing I was kind of worried about, I think this is safe to merge now.

[posva]

Thank you so much!! I will take a look to get it merged next week!

[posva]

Also run CI after rebasing against dev with some extra tests against browsers: https://circleci.com/gh/vuejs/vue-router/tree/fix%2F2606%2Fpoll-leak

@yyx990803 Could you get a look at this one? I want to be sure we are not missing a case that was supposed to be covered by the old poll function

[ronald-d-rogers]

While you guys are all taking a look, a major optimization I thought of the other day was to try to extract and call the entered cbs in the router view and take it out of history completely. Then we wouldn't have to put it in the route object, and all of the entered cb logic would be contained in one place. It's hard for me to tell whether or not everything would still happen in the correct order (and don't know if we care if it differs slightly). Perhaps you guys can say. I've been busy unfortunately so haven't had time to try.

[ronald-d-rogers]

@posva FYI, I spent the time to actually try and do what I said in the above comment, and realized it is not possible. The cb's have to be extracted in confirmTransition when the hooks are called. So please disregard.

[posva]

@ronald-d-rogers thank you for that! I haven't forgotten about this PR and I will try to review it in a few weeks

[posva]

I'm so sorry for the delay @ronald-d-rogers !
I imagine you had a patch running on your side. It's a risky change, so I did prioritize some other bugs and features before getting to it. I will merge it and release it in a patch version soon

[ronald-d-rogers]

@posva: No worries, last thing I want to be is at all responsible for breaking the internet! Just FYI, I did not have a patch running on my end, I just laid out rules to avoid this feature, use workarounds, etc.