Adding a shutdown method to the AttachmentsFileManager #1428
Conversation
app/server/lib/ActiveDoc.ts
Outdated
| @@ -2170,6 +2170,9 @@ export class ActiveDoc extends EventEmitter { | |||
| }; | |||
|
|
|||
| try { | |||
| await safeCallAndWait('attachmentFileManager', | |||
There was a problem hiding this comment.
Is this not a little too early? The doc isn't yet marked as "muted" and disconnected from clients, so we could get parallel activity happening on the doc during this call.
There was a problem hiding this comment.
Moved it after webhooks shutdown.
| @@ -106,6 +110,15 @@ export class AttachmentFileManager { | |||
| this._docPoolId = _docInfo ? getDocPoolIdFromDocInfo(_docInfo) : null; | |||
| } | |||
|
|
|||
| public async shutdown(): Promise<void> { | |||
| if (this._loopAbort.aborted) { | |||
| throw new Error("shutdown already in progress"); | |||
There was a problem hiding this comment.
This could be fine, or it could just skip to awaiting _transferJob? But I expect there's a deliberate reason for throwing an error here?
There was a problem hiding this comment.
Yes, I didn't want to allow calling this method twice. The abort signal is triggered 3 lines below.
|
There are a lot of |
| if (this._loopAbort.aborted) { | ||
| throw new Error("AttachmentFileManager was shut down"); | ||
| } |
There was a problem hiding this comment.
What's the thinking behind specifically erroring in this method, and not others? (E.g _storeFileInLocalStorage).
There was a problem hiding this comment.
Nothing special. I just thought that the external storage is likely to be the problem, not the internal or test.
Long story:
I actually first prepared a full solution here, fix the close method on providers (by calling it during shutdown), added signal for each upload (with some tricks, to abort readable streams, or pipes) and so on. But then I realized that it is too hard to test and to review. So I remove 99% of the code, and just left the simple method that should abort the loop.
There was a problem hiding this comment.
Re-reading this bit of the code, I'd guess the internal storage would have been the issue due to the DocStorage being shutdown and closing the DB connection. I think remote uploads would be fine, as the stores aren't tied to ActiveDoc for their connections .
I think you've guarded against the internal storage / DB access being a problem with the changes to ActiveDoc (as ActiveDoc awaits shutdown for this before closing DocStorage).
I'd be inclined to remove this - I'm not sure it's actually protecting against anything now, and might cause more errors (as I think the transfer would succeed without this guard in place).
Context
During ActiveDoc shutdown process the AttachmentsFileManager's internal loop wasn't interrupted. This prevented server to shutdown gracefully.
Proposed solution
The manager class is now closed properly during ActiveDoc shutdown process. This still can be improved by implementing:
Has this been tested?