Add robust error handling and Sentry tracking for backup extraction #1455
Conversation
| } | ||
| totalBytesToRead -= data.bytesRead; | ||
| } | ||
| endStream(); |
There was a problem hiding this comment.
Do we need endStream here? Given that we have the same in the finally part.
| if ( ! outputStream.destroyed ) { | ||
| outputStream.write( buffer ); | ||
| } else { | ||
| return; | ||
| } |
There was a problem hiding this comment.
| if ( ! outputStream.destroyed ) { | |
| outputStream.write( buffer ); | |
| } else { | |
| return; | |
| } | |
| if ( outputStream.destroyed ) { | |
| return; | |
| } | |
| outputStream.write( buffer ); |
Nitpick: Doing an early return would improve code readability.
| @@ -47,6 +48,17 @@ export class BackupHandlerZip extends EventEmitter implements BackupHandler { | |||
| this.emit( ImportEvents.BACKUP_EXTRACT_START ); | |||
|
|
|||
| return new Promise( ( resolve, reject ) => { | |||
| let extractionFailed = false; | |||
| function failOnce( err: Error, context?: Record< string, unknown > ) { | |||
There was a problem hiding this comment.
| function failOnce( err: Error, context?: Record< string, unknown > ) { | |
| const failOnce = (err: Error, context?: Record<string, unknown>) => { |
Can we use an inline function instead of an arrow function for failOnce?
| const writeStream = fs.createWriteStream( fullPath ); | ||
|
|
||
| const onError = ( err: Error ) => { | ||
| failOnce( err, { fullPath, entry: entry.fileName, filePath: file.path } ); |
There was a problem hiding this comment.
If an error occurs while reading or writing the stream, and the stream remains open, we may be left with lingering streams. I believe the code can benefit from manually closing the stream if it hasn't already been closed when the error occurred. What do you think about it?
if (!readStream.destroyed) {
readStream.destroy();
}
if (!writeStream.destroyed) {
writeStream.destroy();
}
|
@gavande1 thanks for the review and comments! |
Related issues
Proposed Changes
ERR_STREAM_DESTROYEDerrorsTesting Instructions
Note: I was unable to reliably reproduce the original
ERR_STREAM_DESTROYEDerror in testing, but the error handling improvements are defensive and should prevent the issue.Pre-merge Checklist