Adding close method to AuditLogger to free up resources. #1292
Conversation
| @@ -172,15 +207,27 @@ export class AuditLogger implements IAuditLogger { | |||
|
|
|||
| private async _getOrSetStreamingDestinations(event: AuditEvent) { | |||
| const orgId = event.context.site?.id; | |||
| const destinations = await Promise.all([ | |||
There was a problem hiding this comment.
To recreate similar issue just promises just put those lines here and run the DocApi2 test.
new Promise<AuditLogStreamingDestination[] | null>((resolve) => setTimeout(resolve, 10 * 1000, null)),
new Promise<AuditLogStreamingDestination[] | null>((res, reject) => setTimeout(reject, 1, null)),
app/server/lib/AuditLogger.ts
Outdated
| // All log calls should be awaited already and errors should be logged. | ||
| await logCall.catch(() => {}); | ||
| } | ||
| if (this._logsInProgress.length > 0) { |
There was a problem hiding this comment.
Right, I understand why you do that: if the logEventOrThrow method is called while lines 119 to 122 are executed, this should log an error (just like the message suggests). It surprised me a bit (I took some time to understand that despite setting _logsInProgress to [], it may still be populated in other places)
But the tests might hang if such a case occur, don't they?
I have an alternative in mind, would it make sense?
- we introduce a boolean property (
_isClosedfor example) which would indicate if theclosemethod has been called; - in
logEventOrThrow(), if the AuditLogger has been closed, we don't even call the internal method_logEventOrThrowand rather log the message below. This way, nothing is done and we don't have to worry about what happens in this case anyway.
| @@ -12,9 +12,6 @@ describe('ColumnOps.ntest', function() { | |||
| await gu.supportOldTimeyTestCode(); | |||
| await gu.useFixtureDoc(cleanup, "World.grist", true); | |||
| await gu.toggleSidePanel('left', 'close'); | |||
| // The banner appearing mid-test occasionally interferes with the rest of | |||
| // the tests (for some unknown reason), so wait for it to appear first. | |||
| await $('.test-banner-element').wait(); | |||
There was a problem hiding this comment.
This banner was only shown on internal CLI (probably), we will sort it differently.
berhalak
force-pushed
the
close-audit-logger
branch
from
8baeec2 to
f30279b
Compare
app/common/delay.ts
Outdated
| /** | ||
| * Returns a promise that resolves in the given number of milliseconds. | ||
| * (A replica of bluebird.delay using native promises.) | ||
| */ | ||
| export function delay(msec: number): Promise<void> { | ||
| return new Promise<void>((resolve) => setTimeout(resolve, msec)); | ||
| } | ||
|
|
||
| export async function waitToPass(fn: () => MaybePromise<any>, maxWaitMs: number = 2000) { |
There was a problem hiding this comment.
Hmm. A bit unusual to put a test-only method in app? But given that there's nowhere very obvious in tests to put a common library, I guess that's ok. Can you add some documentation about what the method is for (I know there's already undocumented copies of the function lying around, but we should have a higher standard for app/common).
There was a problem hiding this comment.
You're right. Turned out we already have it, so I refactored the old one a bit.
app/common/delay.ts
Outdated
| await delay(10); | ||
| } | ||
| } | ||
| await fn(); |
There was a problem hiding this comment.
Can you add a little note that this is to throw a meaningful error if the fn isn't passing, rather than just for example saying the timeout failed.
There was a problem hiding this comment.
Refactored it to make it more obvious what is going on - and it doesn't need a comment now.
| @@ -960,6 +960,7 @@ export class FlexServer implements GristServer { | |||
| // Do this after _shutdown, since DocWorkerMap is used during shutdown. | |||
| if (this._docWorkerMap) { await this._docWorkerMap.close(); } | |||
| if (this._sessionStore) { await this._sessionStore.close(); } | |||
| if (this._auditLogger) { await this._auditLogger.close(); } | |||
There was a problem hiding this comment.
Thinking about what happens if there are slow audit sinks and this close takes time. Not much we can do. We could have started the close earlier, in parallel with other actions - but perhaps we want to audit those :).
Let's leave it so.
There was a problem hiding this comment.
This will go away with jobs. Then we would just abort it, and resume later.
berhalak
force-pushed
the
close-audit-logger
branch
from
757a779 to
6840680
Compare
|
Sorry for the additional code, there were two additional problems:
|
berhalak
force-pushed
the
close-audit-logger
branch
from
bbf58d9 to
6de01d5
Compare
|
Last commit was just a rebase. |
Context
During
DocApi2server tests we noticed that the mocha process hangs on some resources.Proposed solution
AuditLogger class had two issues which are now fixed:
Promise.all) where awaitedHas this been tested?