fix(ssr): memory leak in poll method #2875
Conversation
|
Ah I see, the E2E tests didn't run locally for some reason. Let me see if I can fix these errors. |
|
Could you rebase against dev? There were a few things in between |
|
I believe I have a solid fix now. Will clean up and commit new E2E test hopefully tomorrow. |
a6379dd to
f340981
Compare
|
@posva There are some edge cases I'm a little worried about, such as the possibility of the |
|
I finally got around to testing a past build that had the memory leaks against this fix, here are the results. E2E TestsAll E2E tests pass for both builds With memory leak: Without memory leak: Memory Leak TestWith memory leakWe 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 leakWe 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. |
|
@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 |
|
@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 ( |
|
@posva I put console log statements in the beginning of |
|
Thank you so much!! I will take a look to get it merged next week! |
|
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 |
|
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. |
|
@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 |
|
@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 |
There was a problem hiding this comment.
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
|
@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. |
Fix #2606.
Unlike #2867, this fix does not depend on the following being merged first: vuejs/vue#9479
Removed
pollmethod, which was causing a memory leak when a Vue app is created per request, which is typical in SSR.The leak happens when:
router-viewis conditioned to appear upon a variable that never becomes true.beforeRouteEnterguard'snextis passed acb:Instead of polling we try to call the
cbinHistory#confirmTransition, otherwise add thecbtoRouteRecord#enteredCbs, and call it instead upon route registration (seecomponents/view.js).I don't know whether or not it is ok to add
enteredCbstoRouteRecord. I don't know whether or not we are ok with mutating the route record inside ofHistory#confirmTransition. I also don't really know if this will cause problems elsewhere outside of runningnpm 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.