SF-3532 Dispose realtime docs when no longer in use

[Nateowami]

This is still work in progress, but I want to put it out here as it appears to be working.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 71.33956% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.40%. Comparing base (ef1f596) to head (2f8bf57).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...re/ClientApp/src/xforge-common/realtime.service.ts	42.85%	23 Missing and 1 partial ⚠️
...diagnostic-overlay/diagnostic-overlay.component.ts	0.00%	14 Missing ⚠️
...e/ClientApp/src/app/shared/project-router.guard.ts	29.41%	12 Missing ⚠️
...nx-insights-panel/lynx-insights-panel.component.ts	7.69%	12 Missing ⚠️
...pture/ClientApp/src/app/core/sf-project.service.ts	76.47%	4 Missing ⚠️
...ate/editor/lynx/insights/lynx-workspace.service.ts	25.00%	3 Missing ⚠️
...ture/ClientApp/src/xforge-common/util/rxjs-util.ts	0.00%	3 Missing ⚠️
...ientApp/src/app/core/translation-engine.service.ts	0.00%	2 Missing ⚠️
.../app/serval-administration/draft-jobs.component.ts	0.00%	2 Missing ⚠️
...nslate/editor/note-dialog/note-dialog.component.ts	80.00%	2 Missing ⚠️
... and 9 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3199      +/-   ##
==========================================
+ Coverage   82.32%   82.40%   +0.08%     
==========================================
  Files         613      613              
  Lines       36879    36982     +103     
  Branches     6023     6032       +9     
==========================================
+ Hits        30359    30474     +115     
+ Misses       5641     5623      -18     
- Partials      879      885       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[marksvc]

Hello @Nateowami ,

Thank you for your work on this!

Here are some comments on the code.

I find positive names to be easier to understand than negative names, when using boolean logic. I suggest to consider renaming DocSubscription.isUnsubscribed$ to isSubscribed$.

Can you explain more about the use of DocSubscription.unsubscribe() instead of using a destroyRef argument to DocSubscription.constructor? It looks like DestroyRef is for some angular classes, and so if we want to use a DocSubscription in a different situation, we might not have a DestroyRef available, and so we would use DocSubscription.unsubscribe?

Rather than provide DocSubscription.unsubscribe for situations where a DestroyRef|Observable was not provided to DocSubscription.constructor, do you think we could always require clients to do one of

• Give a DestroyRef that they have.
• inject a DestroyRef, and pass it to DocSubscription.constructor, and later the DestroyRef gets destroyed when.. when the "corresponding injector is destroyed"; or
• Pass an Observable, and emit from the Observable when the client wants to unsubscribe.
  I'm thinking this would reduce the complexity and variation of the DocSubscription class. (DocSubscription.unsubscribe could be removed.)
  I might have to have a look at what that looks like to decide if I like it better or worse :).

It looks like if we removed DocSubscription.unsubscribe, and instead had clients pass an Observable, that might look something like

// New client field
private readonly bye$ = new Subject<void>();
...
// Pass in constructor
new DocSubscription('DraftGenerationStepsComponent', this.bye$.asObservable())
...
// New client method
wave(): void {
  this.bye$.next();
  this.bye$.complete();
}

I want to mention that we could further reduce the complexity of DocSubscription by changing the constructor destroyRef argument from DestroyRef | Observable<void> to just Observable<void>. That would give some simplification.
Client code might change from

new DocSubscription('DraftGenerationStepsComponent', this.destroyRef)

to something like

new DocSubscription('DraftGenerationStepsComponent', new Observable(subscriber => this.destroyRef.onDestroy(() => subscriber.next())))

That makes the client code more complex just to simplify DocSubscription.constructor by a couple lines, so I'm not too inclined toward it. But I wanted to mention that idea in case it inspires other ideas.

Sometimes when working in TypeScript it seems like it could be useful to have a standard disposable system. In C#, there is an IDisposable interface, and implementing classes have a dispose() method that is called when the class is going away. In this dispose method you can let go of resources that you were holding onto.
If we had any desire to apply a generic disposal system across the frontend TypeScript application, this might be a good place to implement and do it.
AI suggested some disposal patterns that could be applied, which are

• Implement our own Disposable interface with a dispose method like C#.
• Using RxJS Subscription, where we add in each held resource and then in ngOnDestroy or another "dispose" method we let go of the resources.
• Using Angular DestroyRef. I wonder if we can use this with arbitrary classes, not just with Components and Directives.
• And of interest, in the future, using might come to TypeScript.

In C#, the IDisposal.dispose method is automatically called if you are using an object and it goes out of scope. (using someResource = foo()) Unfortunately, only the Angular DestroyRef and in-the-future using patterns above would be as automated (at least where we could not rely on ngOnDestroy). Hmm.
Well, do you have any comments on generic disposal ability in the frontend application?

[marksvc]

@marksvc reviewed 12 of 132 files at r1, 4 of 17 files at r4, 2 of 3 files at r6, 1 of 7 files at r7.
Reviewable status: 34 of 136 files reviewed, 9 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.ts line 64 at r6 (raw file):

    const projectDoc: SFProjectProfileDoc = await this.projectService.getProfile(projectId, docSubscription);
    const result = isParatextRole(projectDoc.data?.userRoles[currentUserDoc.id] ?? SFProjectRole.None);
    docSubscription.unsubscribe();

This is such a good case for using syntax some day :)
(A proposed change to JavaScript.)

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts line 39 at r6 (raw file):

    const textDoc: TextDoc = await this.projectService.getText(
      textDocId,
      new DocSubscription('TextDocService', this.destroyRef)

So for situations like this, would we want to unsubscribe from the doc at the end of the method?

Even more importantly since this service is probably a singleton that might exist for a long time on a given client.

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts line 93 at r6 (raw file):

    if (textDoc?.data != null) {
      throw new Error(`Text Doc already exists for ${textDocId}`);

This would be more pedantic, but it would not be incorrect to here unsubscribe from textDoc just before throwing, right?

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/sf-project.service.ts line 102 at r6 (raw file):

    const docSubscription = new DocSubscription('SFProjectService.isProjectAdmin');
    const projectDoc = await this.getProfile(projectId, docSubscription);
    docSubscription.unsubscribe();

I'm changing this to fetch the data before unsubscribing, in case unsubscribing happens before we look at the data.

Hmm, though after after the realtimedoc is disposed, I suspect we are left with a copy of the data here in projectDoc. Can you comment on that?

It does seem conceptually wrong to have code saying "Okay I'm done with this! Now I'll look at it."

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/sf-project.service.ts line 162 at r6 (raw file):

      [obj<NoteThread>().pathStr(t => t.verseRef.chapterNum)]: chapterNum
    };
    return this.realtimeService.subscribeQuery(NoteThreadDoc.COLLECTION, 'query_note_threads', queryParams, destroyRef);

I puzzled over who was responsible for holding+disposing this for a bit until I realized that (1) we are passing destroyRef, and (2) the method is first also getting destroyRef. :)

---

src/SIL.XForge.Scripture/ClientApp/src/app/connect-project/connect-project.component.ts line 166 at r6 (raw file):

      return;
    }
    this.projectDoc = await this.projectService.subscribe(

Can you explain about the change here from get to subscribe? I expect it is to emphasize that you are getting something that you retain a tie to?

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-query.ts line 15 at r7 (raw file):

export class RealtimeQuery<T extends RealtimeDoc = RealtimeDoc> {
  private _docs: T[] = [];
  private unsubscribe$ = new Subject<void>();

I was adding a comment to unsubscribe$ saying

/** The object is being disposed. */

But when I looked further at how it is being used I decided really I should go ahead and rename the field. I'm mentioning it because maybe I wasn't thinking about the right aspects or nuances of how this should be named.

[marksvc]

@marksvc reviewed 17 of 132 files at r1.
Reviewable status: 49 of 136 files reviewed, 12 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.ts line 704 at r6 (raw file):

      const projectDoc = await this.servalAdministrationService.subscribe(
        projectId,
        new DocSubscription('DraftJobsComponent')

I added this.destroyRef here unless there was a reason it was omitted.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts line 57 at r6 (raw file):

  constructor(
    private readonly projectService: SFProjectService,

Can you explain why CacheService was removed?

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/user.service.ts line 107 at r6 (raw file):

        }
        return from(
          this.realtimeService.onlineQuery<UserDoc>(UserDoc.COLLECTION, 'query_users', merge(filters, queryParameters))

I'm having trouble understanding who holds+disposes of this query. Can you point me to the right thing?

[Nateowami]

I've responded to most of your comments, and will try to get to the others. They're a bit more complicated.

@Nateowami reviewed 1 of 132 files at r1.
Reviewable status: 48 of 136 files reviewed, 13 unresolved discussions (waiting on @marksvc)

---

src/SIL.XForge.Scripture/ClientApp/src/app/connect-project/connect-project.component.ts line 166 at r6 (raw file):

Previously, marksvc wrote…

Can you explain about the change here from get to subscribe? I expect it is to emphasize that you are getting something that you retain a tie to?

This is a method rename. See src/SIL.XForge.Scripture/ClientApp/src/xforge-common/project.service.ts. The implementation calls subscribe internally. Yes, it's about clarifying what's actually happening.

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.ts line 64 at r6 (raw file):

Previously, marksvc wrote…

This is such a good case for using syntax some day :)
(A proposed change to JavaScript.)

It's not impossible to create a usingRealtimeDoc function if we want to.

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/sf-project.service.ts line 102 at r6 (raw file):

Previously, marksvc wrote…

I'm changing this to fetch the data before unsubscribing, in case unsubscribing happens before we look at the data.

Hmm, though after after the realtimedoc is disposed, I suspect we are left with a copy of the data here in projectDoc. Can you comment on that?

It does seem conceptually wrong to have code saying "Okay I'm done with this! Now I'll look at it."

Yeah, that seems like an improvement. Though I can't look at the line you changed and not have it scream "optional chaining" when I read it.

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts line 39 at r6 (raw file):

Previously, marksvc wrote…

So for situations like this, would we want to unsubscribe from the doc at the end of the method?

Even more importantly since this service is probably a singleton that might exist for a long time on a given client.

Correct.

Seems like a good place for try {} finally {}

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts line 93 at r6 (raw file):

Previously, marksvc wrote…

This would be more pedantic, but it would not be incorrect to here unsubscribe from textDoc just before throwing, right?

Seems like another good place for try {} finally {}

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.ts line 704 at r6 (raw file):

Previously, marksvc wrote…

I added this.destroyRef here unless there was a reason it was omitted.

This is a pretty new component. I'm guessing the addition of the DocSubscription was one of your changes, since I don't think I've reconciled changes with this branch since

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts line 57 at r6 (raw file):

Previously, marksvc wrote…

Can you explain why CacheService was removed?

The placement of CacheService in ActivatedProjectService inverts the intended use of the ActivatedProjectService.

Components and services that need to know what project is active are supposed to inject ActivatedProjectService and subscribe to updates, not update this class to push changes to services that want to know about them. The ActivatedProjectService shouldn't even know the CacheService exists.

However, since nothing needs the CacheService, not injecting it in ActivatedProjectService would cause it to never get constructed. Hence it got added to the application initializer.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 257 at r6 (raw file):

Previously, marksvc wrote…

What is the purpose of moving this.docs.delete up here rather than leaving it in onLocalDocDispose?

That is a very good question, and I'm still trying to figure it out.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/user.service.ts line 107 at r6 (raw file):

Previously, marksvc wrote…

I'm having trouble understanding who holds+disposes of this query. Can you point me to the right thing?

For the most part our queries already do get disposed properly, I think (even prior to this PR). One change in this PR is that when a query is disposed, the realtime docs within it now also get disposed if nothing else holds on to them.

The new developer diagnostics will also highlight when realtime queries are not properly disposed.

I'm not sure this one actually does get cleaned up though. Fixing every failure to properly clean up is not a goal of this PR. Providing a system where realtime docs can be disposed (previously it was never) and making it visible which docs and queries are active (so we can see cleanup bugs) are the two main goals.

The addition of query_users means that the developer diagnostics panel can pinpoint this method as a problem, if it is a problem.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 113 at r9 (raw file):

    // Handle documents that currently exist but are in the process of being disposed.
    const docId: string | undefined = doc?.id;
    if (docId != null) {

Why is this checking if docId is null, rather than checking if doc is null? I think my original changes to this method were simpler and easier to reason about.

[marksvc]

@marksvc reviewed 3 of 132 files at r1.
Reviewable status: 51 of 136 files reviewed, 14 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/tabs/editor-tab-persistence.service.ts line 65 at r6 (raw file):

          projectId,
          this.userService.currentUserId,
          new DocSubscription('EditorTabPersistenceService', this.destroyRef)

I imagine that one kind of bug we might start to encounter is that of having the wrong component hold onto the DocSubscription, such that it's let go of before the actual user of the data is done with it. I had to look around for a bit to understand if calling code should be the one to hold onto the DocSubscription here.

---

src/SIL.XForge.Scripture/ClientApp/src/app/users/roles-and-permissions/roles-and-permissions-dialog.component.ts line 54 at r6 (raw file):

  async ngOnInit(): Promise<void> {
    this.onlineService.onlineStatus$.subscribe(isOnline => {

Interesting that this was here, since it looks like it happens with updateFormEditability below. Removed.

[marksvc]

Reviewable status: 50 of 136 files reviewed, 6 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/permissions.service.ts line 64 at r6 (raw file):

Previously, Nateowami wrote…

It's not impossible to create a usingRealtimeDoc function if we want to.

Right. Okay. We can see if that seems like a good idea as we spend time using this.

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/sf-project.service.ts line 102 at r6 (raw file):

Previously, Nateowami wrote…

Yeah, that seems like an improvement. Though I can't look at the line you changed and not have it scream "optional chaining" when I read it.

Yes :). I have changed it to use optional chaining.

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts line 39 at r6 (raw file):

Previously, Nateowami wrote…

Correct.

Seems like a good place for try {} finally {}

I have implemented that.

---

src/SIL.XForge.Scripture/ClientApp/src/app/core/text-doc.service.ts line 93 at r6 (raw file):

Previously, Nateowami wrote…

Seems like another good place for try {} finally {}

I wanted to discuss this a bit as we learn how to use our new systems. At first I was thinking I could call subscriber.unsubscribe(), like this:

    try {
      let textDoc: TextDoc = await this.projectService.getText(textDocId, subscriber);

      if (textDoc?.data != null) {
        throw new Error(`Text Doc already exists for ${textDocId}`);
      }
    } finally {
      subscriber.unsubscribe(); // <---
    }
    data ??= { ops: [] };
    ...

But that's not good if the caller is using subscriber for a bunch of things. (Which is not the case in this situation, but it could be. For example, the caller could do something like

const subscriber = new DocSubscription('foo');
const a = await textDocService.createTextDoc(textDocId, subscriber); // <-- using it here
const b = await textDocService.createTextDoc(differentId, subscriber); // <-- using the same subscriber again

and so if when createTextDoc(differentId is called, TextDocService ran subscriber.unsubscribe(), we would be ending what is also attached to the request for textDocId, not just what is attaching for differentId.)

So instead, suppose in TextDocService.createTextDoc we make a new docSubscription that we can pass to this.projectService.getText(), that we can run docSubscription.unsubscribe() on without interfering with other things that the caller is doing. And we could tie it to the incoming DocSubscription object:

    const docDone = new Subject<void>();
    subscriber.isUnsubscribed$.subscribe(() => {
      docDone.next();
      docDone.complete();
    });
    const docSubscription = new DocSubscription('TextDocService.createTextDoc', docDone);
    try {
      const gottenTextDoc: TextDoc = await this.projectService.getText(textDocId, docSubscription);
      if (gottenTextDoc?.data != null) throw new Error(`Text Doc already exists for ${textDocId}`);
    } finally {
      docSubscription.unsubscribe();
    }

    data ??= { ops: [] };
    ...

or another way:

    const docSubscription = new DocSubscription('TextDocService.createTextDoc');
    subscriber.isUnsubscribed$.subscribe(() => docSubscription.unsubscribe());
    try {
      const gottenTextDoc: TextDoc = await this.projectService.getText(textDocId, docSubscription);
      if (gottenTextDoc?.data != null) throw new Error(`Text Doc already exists for ${textDocId}`);
    } finally {
      docSubscription.unsubscribe();
    }

    data ??= { ops: [] };
    ...

The changes I made to do that helped me realize we can further simplify this since the result of projectService.getText is never returned to the caller anyway. But I wanted to show what I came up with above.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.ts line 704 at r6 (raw file):

Previously, Nateowami wrote…

This is a pretty new component. I'm guessing the addition of the DocSubscription was one of your changes, since I don't think I've reconciled changes with this branch since

Could be :)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts line 57 at r6 (raw file):

Previously, Nateowami wrote…

The placement of CacheService in ActivatedProjectService inverts the intended use of the ActivatedProjectService.

Components and services that need to know what project is active are supposed to inject ActivatedProjectService and subscribe to updates, not update this class to push changes to services that want to know about them. The ActivatedProjectService shouldn't even know the CacheService exists.

However, since nothing needs the CacheService, not injecting it in ActivatedProjectService would cause it to never get constructed. Hence it got added to the application initializer.

That is helpful, thank you.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 113 at r9 (raw file):

Previously, Nateowami wrote…

Why is this checking if docId is null, rather than checking if doc is null? I think my original changes to this method were simpler and easier to reason about.

I made changes because there was a ! non-null assertion operator, and I continue to see many errors happen on SF that say there was an exception because we tried to inspect a field of an undefined object. But then when I was making the changes I made a mistake. I have corrected it.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/user.service.ts line 107 at r6 (raw file):

Previously, Nateowami wrote…

For the most part our queries already do get disposed properly, I think (even prior to this PR). One change in this PR is that when a query is disposed, the realtime docs within it now also get disposed if nothing else holds on to them.

The new developer diagnostics will also highlight when realtime queries are not properly disposed.

I'm not sure this one actually does get cleaned up though. Fixing every failure to properly clean up is not a goal of this PR. Providing a system where realtime docs can be disposed (previously it was never) and making it visible which docs and queries are active (so we can see cleanup bugs) are the two main goals.

The addition of query_users means that the developer diagnostics panel can pinpoint this method as a problem, if it is a problem.

Thank you. And what this PR is doing is significantly better than what we had before!

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-doc.ts line 46 at r6 (raw file):

Previously, Nateowami wrote…

Calling destroyRef.onDestroy on a DestroyRef that has already been destroyed causes the error. It seems like this if statement should be:

if (isNG0911Error(error)) this.complete();
else throw error;

Thank you. I have adjusted the code.

[Nateowami]

Reviewable status: 49 of 136 files reviewed, 6 unresolved discussions (waiting on @marksvc)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 113 at r9 (raw file):

Previously, marksvc wrote…

I made changes because there was a ! non-null assertion operator, and I continue to see many errors happen on SF that say there was an exception because we tried to inspect a field of an undefined object. But then when I was making the changes I made a mistake. I have corrected it.

Here's what the original implementation was:

  async get<T extends RealtimeDoc>(collection: string, id: string, subscriber: DocSubscription): Promise<T> {
    const key = getDocKey(collection, id);
    let doc = this.docs.get(key);

    // Handle documents that currently exist but are in the process of being disposed.
    if (doc != null && this.disposingDocIds.has(doc.id)) {
      console.log(`Waiting for document ${key} to be disposed before recreating it.`);
      await lastValueFrom(this.disposingDocIds.get(doc.id)!);
      // Recursively call this method so if multiple callers are waiting for the same document to be disposed, they will
      // all get the same instance.
      return await this.get<T>(collection, id, subscriber);
    }

    if (doc == null) {
      const RealtimeDocType = this.typeRegistry.getDocType(collection);
      if (RealtimeDocType == null) {
        throw new Error('The collection is unknown.');
      }
      doc = new RealtimeDocType(this, this.remoteStore.createDocAdapter(collection, id));
      if (doc.id == null) {
        throw new AppError('Document could not be created.', {
          collection: collection,
          id: id ?? 'undefined'
        });
      }
      this.docs.set(key, doc);
      this.docLifecycleMonitor.docCreated(getDocKey(collection, id), subscriber.callerContext);
    }
    doc.addSubscriber(subscriber);

    return doc as T;
  }

I agree that we should generally avoid non-null assertions, but in situations where it's simple to tell that it's correct, they don't bother me. I suppose we could avoid calling disposingDocIds.has and just call disposingDocIds.get and then check if the value is nullish or not.

[marksvc]

Reviewable status: 49 of 136 files reviewed, 5 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.spec.ts line 83 at r7 (raw file):

Previously, Nateowami wrote…

This pattern of constructing an object and then having to call init on it screams "factory function" to me. Maybe we could make the constructor private, and then do const env = await TestEnvironment.create(false);, here and in other tests?

I have done that here. Because init is now private, it should be moved down to lower in the file below the public methods. But for ease of review, I have left it where it is.

I did similarly to question-dialog.component.spec.ts, question-dialog.service.spec.ts, draft-sources.component.spec.ts, and lynx-workspace.service.spec.ts.

AppComponent contsructor and init are different and I left them.

[marksvc]

@marksvc reviewed 2 of 132 files at r1.
Reviewable status: 51 of 136 files reviewed, 7 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 113 at r9 (raw file):

Previously, Nateowami wrote…

Here's what the original implementation was:

  async get<T extends RealtimeDoc>(collection: string, id: string, subscriber: DocSubscription): Promise<T> {
    const key = getDocKey(collection, id);
    let doc = this.docs.get(key);

    // Handle documents that currently exist but are in the process of being disposed.
    if (doc != null && this.disposingDocIds.has(doc.id)) {
      console.log(`Waiting for document ${key} to be disposed before recreating it.`);
      await lastValueFrom(this.disposingDocIds.get(doc.id)!);
      // Recursively call this method so if multiple callers are waiting for the same document to be disposed, they will
      // all get the same instance.
      return await this.get<T>(collection, id, subscriber);
    }

    if (doc == null) {
      const RealtimeDocType = this.typeRegistry.getDocType(collection);
      if (RealtimeDocType == null) {
        throw new Error('The collection is unknown.');
      }
      doc = new RealtimeDocType(this, this.remoteStore.createDocAdapter(collection, id));
      if (doc.id == null) {
        throw new AppError('Document could not be created.', {
          collection: collection,
          id: id ?? 'undefined'
        });
      }
      this.docs.set(key, doc);
      this.docLifecycleMonitor.docCreated(getDocKey(collection, id), subscriber.callerContext);
    }
    doc.addSubscriber(subscriber);

    return doc as T;
  }

I agree that we should generally avoid non-null assertions, but in situations where it's simple to tell that it's correct, they don't bother me. I suppose we could avoid calling disposingDocIds.has and just call disposingDocIds.get and then check if the value is nullish or not.

Thank you; I needn't ascribe the ! coming in to the changes you made :). A recent change I made is doing what you suggest, with just disposingDocIds.get.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts line 112 at r12 (raw file):

    const projectDoc: SFProjectProfileDoc = await this.projectService.getProfile(
      projectId,
      new DocSubscription('ActivatedProjectService', this.destroyRef)

Hmmm. I wonder if any requestors will have a reference to the SFProjectProfileDoc, and use it for some good reason after the current/active project changes, but then the SFProjectProfileDoc is destroyed out from under them. Perhaps never.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 51 at r12 (raw file):

      for (const text of project.data.texts) {
        for (const chapter of text.chapters) {
          if (this.currentProject.projectId != null && this.currentProject.projectId !== project.id) return;

Is this line here in case the active/current project changes while loadAllChapters is running? Okay, I see that matches functionality that was present before but implemented in a different way. Why do you also check if this.currentProject.projectId != null as part of it? Based on the code in the contsructor, I am expecting it is because when a project changes, currentProject.projectId changes from "previousProjectId" to undefined to "newProjectId". Is that so + why?

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 55 at r12 (raw file):

          const textDocId = new TextDocId(project.id, text.bookNum, chapter.number, 'target');
          if (await this.permissionsService.canAccessText(textDocId)) {
            this.subscribedTexts.push(await this.projectService.getText(textDocId, docSubscription));

Previously we were not keeping a reference to the TextDoc (such as by storing it in subscribedTexts). Was this causing it to not be adequately cached?

I don't think we will need to keep the reference to the TextDoc in order to prevent it from being destroyed, since our docSubscription is still hanging around until we .unsubscribe() from it.

Can you explain a bit more about what the purpose is of recording the TextDoc objects in this.subscribedTexts?

[marksvc]

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.spec.ts line 1 at r12 (raw file):

// import { NgZone } from '@angular/core';

I am working on writing a test suite for the new CacheService.

[Nateowami]

Reviewable status: 50 of 136 files reviewed, 8 unresolved discussions (waiting on @marksvc)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 51 at r12 (raw file):

Previously, marksvc wrote…

Is this line here in case the active/current project changes while loadAllChapters is running? Okay, I see that matches functionality that was present before but implemented in a different way. Why do you also check if this.currentProject.projectId != null as part of it? Based on the code in the contsructor, I am expecting it is because when a project changes, currentProject.projectId changes from "previousProjectId" to undefined to "newProjectId". Is that so + why?

currentProject should be renamed to activatedProject for clarity. If activatedProject.id == null, it would indicate no project is currently active (such as when on the my projects page). We wouldn't want to stop caching until a new project is activated.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 55 at r12 (raw file):

Previously, marksvc wrote…

Previously we were not keeping a reference to the TextDoc (such as by storing it in subscribedTexts). Was this causing it to not be adequately cached?

I don't think we will need to keep the reference to the TextDoc in order to prevent it from being destroyed, since our docSubscription is still hanging around until we .unsubscribe() from it.

Can you explain a bit more about what the purpose is of recording the TextDoc objects in this.subscribedTexts?

Thanks for noticing this. I think there was a point where I was keeping track of them so we could dispose them. This PR then progressed well beyond that point, to where docs clean up themselves. Looking at uses of subscribedTexts in this file:

• It's defined
• It's reset to []
• It has elements pushed to it

At no point do we read elements out of it. Which means it can be removed. That was a good catch.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.spec.ts line 1 at r12 (raw file):

Previously, marksvc wrote…

I am working on writing a test suite for the new CacheService.

Thanks. I had forgotten I'd commented these out.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts line 112 at r12 (raw file):

Previously, marksvc wrote…

Hmmm. I wonder if any requestors will have a reference to the SFProjectProfileDoc, and use it for some good reason after the current/active project changes, but then the SFProjectProfileDoc is destroyed out from under them. Perhaps never.

I think you're misreading the situation.

The ActivatedProjectService is always the subscriber that holds on to the SFProjectProfileDoc. The service will live for the life of the application, so they'll never get disposed. Which is OK. Users of the ActivatedProjectService basically "borrow" the project doc from the service, and don't have to "buy" the doc themselves, since the ActivatedProjectService owns it permanently.

If the service didn't take ownership of it, when you go from one page to another, the page you're leaving would release its hold on the document milliseconds before the next page requested it, leading to destroying and recreating docs. This trashing in some cases can happen thousands of times per second, in the worst cases (such as when calling a permission check on each chapter, if that check loads and destroys a doc each time).

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/models/realtime-doc.ts line 253 at r6 (raw file):

Previously, marksvc wrote…

The comment I added to this and the method below sounded pretty silly (since if there is a doc subscription, presumably it is subscribed :)

Hmm, some alternate terminology for DocSubscription might be:

DocHandle, or
DocRequest, or
DocHold

And instead of isUnsubscribed, it could be

active, or
connected, or
complete, or
ended, or
terminated

Then we'd be seeing things like

if (docHandle.active)
  count++

if (!docHandle.active)
  this.dispose()

Or comments like

/** Number of doc handles that are still active. */

DocSubscription isn't a bad name. And it could be paired with active instead of isUnsubscribed. So we'd have a comment like

/** Number of doc subscriptions that are still active. */

Or the field could be complete to be symmetric with isUnsubscribed.

Does any of this alternate terminology sound preferable?

We can probably get rid of docSubscriptionsCount now, since we now clean up docs instantly when they're no longer in use. This was added temporarily when I had implemented logic to track what was using documents, but was still experimenting with how they are cleaned up (for example, I had a garbage collection approach where it would look for documents to be cleaned up every few seconds, which was one approach to avoid repeatedly destroying and recreating documents).

I don't think I really like the idea of changing the name, even though I've never liked DocSubscription as a name. I think it's important to convey the fact that we're working with a subscription here; i.e. the type of thing that one ought to unsubscribe from when it's no longer needed. (Also, it wouldn't be hard to do a rename in the future if we come up with a better description)

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 112 at r13 (raw file):

    if (doc != null) {
      // Handle documents that currently exist but are in the process of being disposed.
      const docBeingDisposed: Subject<void> | undefined = this.docsBeingDisposed.get(doc.id);

This variable seems to me to be misnamed. It's not getting a doc. It's getting a Subject<void> that will complete when the doc has been fully cleaned up.

Also, we usually end observables with $ to indicate their type. I think this convention makes sense, since while a variable name should usually give you some kind of hint about its type (for example, count is probably a number, doc is probably a realtime doc), an observable for a count being named count would make it appear to be a number. Also, usually after you observe the thing, you get the value inside. So if you have an observable for a count named count, you can't subscribe to it, get the emitted value, and call that count. So the pattern of count$.subscribe(count => console.log(count)) works pretty well. Otherwise you write count.subscribe(helpWhatDoICallThisVariable => console.log(helpWhatDoICallThisVariable))

[marksvc]

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 55 at r12 (raw file):

Previously, Nateowami wrote…

Thanks for noticing this. I think there was a point where I was keeping track of them so we could dispose them. This PR then progressed well beyond that point, to where docs clean up themselves. Looking at uses of subscribedTexts in this file:

• It's defined
• It's reset to []
• It has elements pushed to it

At no point do we read elements out of it. Which means it can be removed. That was a good catch.

Okay. Working on that.

[marksvc]

Reviewable status: 50 of 136 files reviewed, 7 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.spec.ts line 1 at r12 (raw file):

Previously, Nateowami wrote…

Thanks. I had forgotten I'd commented these out.

Here is a test suite.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/activated-project.service.ts line 112 at r12 (raw file):

Previously, Nateowami wrote…

I think you're misreading the situation.

The ActivatedProjectService is always the subscriber that holds on to the SFProjectProfileDoc. The service will live for the life of the application, so they'll never get disposed. Which is OK. Users of the ActivatedProjectService basically "borrow" the project doc from the service, and don't have to "buy" the doc themselves, since the ActivatedProjectService owns it permanently.

If the service didn't take ownership of it, when you go from one page to another, the page you're leaving would release its hold on the document milliseconds before the next page requested it, leading to destroying and recreating docs. This trashing in some cases can happen thousands of times per second, in the worst cases (such as when calling a permission check on each chapter, if that check loads and destroys a doc each time).

Thank you. Right, the DocSubscription is using ActivatedProjectService's this.destroyRef. Thank you.

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts line 112 at r13 (raw file):

Previously, Nateowami wrote…

This variable seems to me to be misnamed. It's not getting a doc. It's getting a Subject<void> that will complete when the doc has been fully cleaned up.

Also, we usually end observables with $ to indicate their type. I think this convention makes sense, since while a variable name should usually give you some kind of hint about its type (for example, count is probably a number, doc is probably a realtime doc), an observable for a count being named count would make it appear to be a number. Also, usually after you observe the thing, you get the value inside. So if you have an observable for a count named count, you can't subscribe to it, get the emitted value, and call that count. So the pattern of count$.subscribe(count => console.log(count)) works pretty well. Otherwise you write count.subscribe(helpWhatDoICallThisVariable => console.log(helpWhatDoICallThisVariable))

Thank you.
Yes, thank you for the reminder and reasoning about using $ for Observables.
I have adjusted it.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/cache-service/cache.service.ts line 11 at r12 (raw file):

// In production this should be true, but when testing doc cleanup it may be useful to set to false an observe behavior
const KEEP_PRIOR_PROJECT_CACHED_UNTIL_NEW_PROJECT_ACTIVATED = true;

I'm removing this to clean the file, but we can get similar functionality by inserting this.uncache() at the top of the constructor's subscribe handler.