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 1 of 132 files at r1, 1 of 9 files at r2, 1 of 3 files at r6.
Reviewable status: 28 of 136 files reviewed, 6 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-text/checking-text.component.spec.ts line 214 at r6 (raw file):

Previously, Nateowami wrote…

It looks like this when is identical to the one above, meaning it completely overrides the one above. (I may have even written this myself; I don't remember)

I removed one and adjusted the other to be more flexible.

---

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

Previously, Nateowami wrote…

Can this be removed now?

Yes; I don't need it. Removed.

---

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

Previously, Nateowami wrote…

We should probably change this to something like RealtimeQuery/${this.name}. What do you think?

(and same lower in the file)

Sounds good. I added that.

---

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

    this.disposingDocIds.set(doc.id, new Subject<void>());
    this.docLifecycleMonitor.docDestroyed(getDocKey(doc.collection, doc.id));
    this.docs.delete(getDocKey(doc.collection, doc.id));

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

---

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

      else destroyRef.subscribe(() => this.complete());
    } catch (error) {
      if (!isNG0911Error(error)) throw error;

It seems odd to me to need to defend against "NG0911: View has already been destroyed" here. Though I'm a bit surprised for there to be errors here anyway. Were you finding that when you construct DocSubscription objects they throw errors, such as NG0911?

---

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

  }

  get docSubscriptionsCount(): number {

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?

[marksvc]

I have been doing some more review as well.

[Nateowami]

@Nateowami reviewed 2 of 7 files at r7.
Reviewable status: 26 of 136 files reviewed, 5 unresolved discussions (waiting on @marksvc)

---

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

Previously, marksvc wrote…

It seems odd to me to need to defend against "NG0911: View has already been destroyed" here. Though I'm a bit surprised for there to be errors here anyway. Were you finding that when you construct DocSubscription objects they throw errors, such as NG0911?

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;

---

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

Previously, marksvc wrote…

Yes; I don't need it. Removed.

@marksvc You're still dissenting on resolving this thread.

---

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

    it('should display "No question" message', fakeAsync(async () => {
      const env = new TestEnvironment();
      await env.init(false);

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?

[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?