memory leak category page

[gibkigonzo]

closes #3805

Short description and why it's useful

• fixes in storage.ts - when set/get value from cache it should always be without reference
• in vuex we can keep only products sku as reference to products data in non reactive state. This will prevent of making observable objects with every commit and then it's easier for GC to clear it.

Which environment this relates to

Check your case. In case of any doubts please read about Release Cycle

• Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
• RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
• Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Upgrade Notes and Changelog

• No upgrade steps required (100% backward compatibility and no breaking changes)
• I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

IMPORTANT NOTICE - Remember to update CHANGELOG.md with description of your change

Contribution and currently important rules acceptance

• I read and followed contribution rules

[pkarw]

Wow, @gibkigonzo nice one's!

[gibkigonzo]

update 8ea7f81
I removed Vue.prototype.$ssrRequestContext. I think we shouldn't share request context between each request. This can be accessed by other request which can make problem for GC to quick data collection. Instead I've added $cacheTags to prototype. We only need this and I think we should reveal only what is needed. Also I've added clearing from context server and meta data. This pr will make GC faster but I didn't found any big memory leak in ssr.

[pkarw]

@gibkigonzo I think that $ssrRequestContext isn't shared anyway as we're using runInNewContext flag set to true in the SSR renderer options. The context is (should be) accesible via Vue.prototype.$ssrContext anyway - the only difference is that it's read only property set by Vue SSR renderer. Our context has some additional features https://docs.vuestorefront.io/guide/core-themes/layouts.html#switching-off-layout-and-injecting-dynamic-content + https://ssr.vuejs.org/guide/head.html)

$ssrrequestContext is not being widely used - as most cases are limited to use the context from within asyncData where the context is passed as well.

The thing is that cacheTags are passed back end forth betweeen core/scripts/server.js and Vuex actions by this reference.

So I'd rather keep it as it was + maybe clear it out (delete or null) after request is finished (?)

[pkarw]

Please do some QA with the config.server. useOutputCacheTagging set to true and make sure the cache tags are still passed back via context/$ssrServerContext (it's the same object, used for the communication from Vue layer to server layer): https://github.com/DivanteLtd/vue-storefront/blob/19b46b8dbf6a0e2e3e6bdc3b43dad38b4cbbffaa/core/scripts/server.js#L192

[gibkigonzo]

It is shared because node keep cached module between requests. It's easy to test, for example add in server-entry.ts

  if (!Vue.prototype.$increment) {
    Vue.prototype.$increment = 1
  }
  Vue.prototype.$increment = Vue.prototype.$increment + 1
  console.log(Vue.prototype.$increment)

As I understand runInNewContext run every module in new context but modules are reused and initialized every time in new context. So if we add something to module prototype it will be shared

[pkarw]

@gibkigonzo nice observation. OK so ssr.clearContext is surely the right approach :)

[gibkigonzo]

I tested config.server. useOutputCacheTagging and it works fine. cacheTags is still shared between server.ts and server-entry.ts

[andrzejewsky]

great work! 👍 👍

[sebastianrothbucher]

Just did some load testing - the main issue with Vue.prototype.$ssrRequestContext.server.response (used e.g. in core/modules/url/router/beforeEach.ts) is prone to race conditions as it's the response last set before we have our response, poss. by some other call to the prerender. So it's hugely misleading (and also still used). I solved it by calling next(new Err(...)) and catching the error in server.js - could do a PR on this