fix: fix memory leak on SSR

[ValentinFunk]

Checklist

• Issue number for this PR: TBD
• Docs included?: no - API did not change
• Test units included?: yes
• In a clean directory, yarn install, yarn test run successfully? Yes + manual testing in larger app (mainly auth/database)

Description

This is bug fix to fix an issue where running SSR with Angularfire would cause a memory leak, preventing the Application Module from being collected.

The leak was caused by
a) subscriptions leaking in the implementation of keepUnstableUntilFirst (https://github.com/angular/angularfire/blob/master/src/core/angularfire2.ts#L19, https://github.com/angular/angularfire/blob/master/src/core/angularfire2.ts#L54)
b) References to the subscriber kept around in the noop function

Explanation of the fix and approach

When investigating this I found that to solve the leak a lot of "null" setting and more hacks on the code were required. I decided to try and improve the hack (https://github.com/angular/angularfire/blob/master/src/core/angularfire2.ts#L13) by makeing it a bit more idiomatic.

To make universal work correctly the following is needed:

1. Make sure that firebase library calls are done outside the Angular Zone. Otherwise the zone doesn't stabilize due to keepalive timers and websockets that are firebase opens.
2. When in SSR, make sure that the angular Zone is blocked by a macrotask until the observable emits an item, completes or errors. This makes sure that SSR pages don't finish rendering prematurely, before data has arrived from firebase.

1) Firebase library calls done outside the Angular Zone

The code was slightly refactored to allow passing an optional Scheduler as parameter to the library factory methods. This allows to easily specify the scheduler used for emissions of items from e.g. fromRef. This is in line with the RxJS api (e,g. fromPromise(promise, scheduler?).

By using subscribeOn combined with a Scheduler that schedules work outside of the Angular Zone, the behavior can easily be achieved. An implementation of such a scheduler has been added as "ZoneScheduler".

Related Tests:

• https://github.com/Kamshak/angularfire/blob/5a9550c993bc4ecf512c3ed5a4a2c25390d47009/src/database/observable/fromRef.spec.ts#L59
• https://github.com/Kamshak/angularfire/blob/5a9550c993bc4ecf512c3ed5a4a2c25390d47009/src/database/observable/fromRef.spec.ts#L44

2) Preventing Zone from stabilizing until first item is emitted

In order to solve the memory leak and make it difficult to accidentally introduce a new one, the approach is to

• use pipeable operators where possible, this reduces the complexity needed to manage subscriptions
• split functionality into small, independent units (Scheduler, operator, operator factory)

The gist is this:

  return function keepUnstableUntilFirst<T>(obs$: Observable<T>): Observable<T> {
    const inCorrectZones$ = obs$.pipe(
      // Run the observable creation outside the angular zone
      subscribeOn(schedulers.outsideAngular),
      // Run operators inside the angular zone (e.g. side effects via tap())
      observeOn(schedulers.insideAngular)
    );

    if (isPlatformServer(platformId)) {
      return inCorrectZones$.pipe(
        blockUntilFirst(schedulers.ngZone)
      );
    } else {
      return inCorrectZones$;
    }
  }

Full, commented implementation: https://github.com/angular/angularfire/blob/5a9550c993bc4ecf512c3ed5a4a2c25390d47009/src/core/angularfire2.ts

These remaining changes from the second commit are only chore work/refactoring to make sure the Schedulers are available where needed.

Testing

Comprehensive testing was added to verify the functionality at each level (the intended behavior hasn't changed from the description in the PR). Most notably keepUnstableUntilFirstFactory now has a test that verfies that it creates a macrotask until the first emission, and removes it after.

ZoneScheduler
✔ should schedule all tasks asynchronously
✔ should execute the scheduled work inside the specified zone
✔ should execute nested scheduled work inside the specified zone

keepUnstableUntilFirstFactory
✔ should subscribe outside angular and observe inside angular (server)
✔ should subscribe outside angular and observe inside angular (browser)
✔ should block until first emission on server platform
✔ should not block on client platform

fromRef
✔ should take a scheduler
✔ should schedule completed and error correctly

Updated universal-test app

In the third commit the universal-test app was updated to Angular 6 to make it work with @angular/fire again. The package.json command has been updated as well, however I did not add it to the travis file (not sure if you would like it tested on CI). No functional changes, testing works again and renders a html file.

NPM version bumped to 5.2.4*
Just a heads up - this was mainly for manual testing with yarn pack - suggesting a patch or minor bump. Also done in 2nd commit.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[ValentinFunk]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[jamesdaniels]

@kamshak thanks so much for this PR! Just skimming through it looks like it cleans up my Zone hacks quite a bit & goes further down the path that I was starting to head down, before I ran out of steam. Great job!

I'm going to dogfood this a little and review, obviously a lot of churn, though low risk IMO as we already had bugs here. Happy to bring in as a patch.

[ValentinFunk]

Sounds great 👍 Thanks for considering the PR

I'll be adding an update to the branch here, we've found an issue with the Scheduler approach when reusing the QueueScheduler as a base and nested scheduling sometimes the event can end up inside the Angular Zone. Will be adding a fix + test for this.

[ValentinFunk]

Match firestore behavior with rtdb behavior (same default scheduling). The scheduler is always specified to use the ZoneScheduler when using the normal Angularfire API, this is just to cover the possibility of someone using fromRef directly (to avoid a breaking change for this case).

[ValentinFunk]

Summary of changes in last commit:

1. Added more tests to verify correct behavior
   Comprehensive testing was added to verify the functionality at each level (the intended behavior hasn't changed from the description in the PR). Most notably keepUnstableUntilFirstFactory now has a test that verfies that it creates a macrotask until the first emission, and removes it after.

ZoneScheduler
✔ should schedule all tasks asynchronously
✔ should execute the scheduled work inside the specified zone
✔ should execute nested scheduled work inside the specified zone
keepUnstableUntilFirstFactory
✔ should subscribe outside angular and observe inside angular (server)
✔ should subscribe outside angular and observe inside angular (browser)
✔ should block until first emission on server platform
✔ should not block on client platform
fromRef
✔ should take a scheduler
✔ should schedule completed and error correctly

2. Bug Fixes

• (keepUnstableUntilFirstFactory) Fixed an issue where nested subscriptions could be scheduled in the wrong zone.
• (fromRef) Made sure that all new emissions (events from firebase) are scheduled correctly
• (fromRef) Fixed completed and error notifications not being scheduled using the specified scheduler
• (ZoneScheduler) Fixed delegate scheduler to schedule async by default

With the tests added the behaviour should now be fully covered and refactoring/modifying this implementation can be done confidently.

[jamesdaniels]

@kamshak awesome. Appreciate the test coverage and the discovery / tightening up of those scheduler issues. Honestly, I hadn't had the time to use in my test project before the holiday, so I hadn't noticed.

This is looking pretty solid to me. I'm going to give it an earnest spin later today and will let you know how it goes; barring anything obvious I'm feeling good since this is an obvious a improvement on what's currently out there. Will try to get it merged this week.

[jamesdaniels]

FYI I've been starting to leave comments for anything I can drop in the next major, mostly older typescript work arounds. // SEMVER drop any when we upgrade typescript in the next major or something along those lines.

I'm aiming to break off from older versions of typescript, ng, and firebase alongside ng 9.

[jamesdaniels]

Also feel free to suggest any API breaks that would make maintainability easier too, as you saw I already had two different styles of zone patching I was supporting.

[jamesdaniels]

I'm ok with using private APIs in the tests but is there a way we can go public? Are there any changes coming up in 9 in regard to these constants?

[jamesdaniels]

Not a big deal TBH, a lot more maintainable than stubbing isBrowser/Server

[jamesdaniels]

Can we type these rather than lean on any?

[ValentinFunk]

Thanks a ton for taking a look at this and for your comments @jamesdaniels . I found two more issue in the implementation that I want to try fixing on the weekend, basically:

• it doesn't clear the zone block if you subscribe to a firebase ref but then unsubscribe before the first emission (can happen quite easily if you switchMap away for example)
• Angular has an issue (intended behavior?) where ngZone doesn't check for Zone stable when a task is cancelled - only on invoke. Will create an issue in angular to clarify if this is intended. We can work around this quite easily by invoking the task instead of cancelling (as was done before)

Will have an update on the weekend. We're also doing a larger round of manual testing on our app where I'm running this fork in the last week as well :)

[jamesdaniels]

@kamshak how did the last round of testing go, any updates?

[ValentinFunk]

Sorry, been a bit buried and forgot to update 😄

Testing went generally well, fixed the issue of unsubscribe before first emission and implemented the workaround (invoke instead of cancel to make sure angular zone stabilizes). Unit Test is added for this as well. It's working fine on all of our SSR routes and on normal app use during our testing. I'll push these changes tomorrow.

I did discover one issue though and I'm not really sure if that is in our app code or if it's in the changes here. Basically as soon as I start to render multiple pages (or the same one) concurrently in the same process, some of them will fail to stabilize the zone (and time out). The weird thing is that it starts happening already consistently with a concurrency of 2, however with a concurrency of 10 still only 1-2/10 renders will fail. This does happen as well if we use the current angularfire version as well, but less frequently.

Right now I suspect it could either be something specific to our app (there is some manual re-scheduling to different zones e.g. that could be causing it) or an issue where firebase might be caching some data.

My plan is to create a smallish app to see if it reproduces there. If it does I would need to do a bit of digging, I've tried logging all Zone tasks and blocks which didn't really give any insight.

[jamesdaniels]

@kamshak ok, thanks for the update. Good to know on that one issue, I've seen something similar there FWIW. We can keep investigating outside this PR.

One you push those last changes up I'll get this merged in and cut as a patch.

[ValentinFunk]

I've pushed the changes, one important note is that there is a breaking change now:
fromRef no longer multicasts by default (e.g. share()). This is now done in keepUnstableUntilFirst, which was needed because otherwise there were issues where the zone blocking would take place once per subscription, which could cause some stuck blocking issues. An alternative would be to share() twice.

This doesn't change anything for users that are not directly using fromRef since in all of the angularfire (database, firestore, ...) classes the zone block operator is called.

I've done some more testing on the other issue where concurrency causes issues as well. It seems that with the changes here it happens much faster than before - I don't know why this is the case though.

[jamesdaniels]

Whoops, broke the build with the merge commit; since I'm using the older stuff in analytics, etc. Going to pull into a working branch & merge today.