FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

Error props canRetry and retryAfterSeconds should not be set on arbitrary error objects via any

Open markfields opened this issue 4 years ago • 2 comments

Here's an example:

https://github.com/microsoft/FluidFramework/blob/main/packages/drivers/odsp-driver/src/odspDocumentStorageManager.ts#L542-L551

The problem is that elsewhere in the code (the normalizeError function) we make the assumption that the only properties that we can safely preserve on an untrusted error object are stack and message. So if an untrusted error gets stamped with canRetry, and then it is later normalized (for the purpose of supporting privacy-safe logging props to be added), then canRetry will be lost.

Instead of setting error.canRetry = true directly on an any type error, we can do normalizeError(error, { props: { canRetry: true } });. This will give a normalized error annotated with canRetry and ready to receive further annotation without losing any key props. (note: normalizeError is idempotent so it can be called again later and will return the error unchanged)

markfields avatar Jan 13 '22 20:01 markfields

This relates to discussions about having errors be returned through our APIs rather than thrown. Then we wouldn't be having to deal with errors in catch blocks which are any typed.

One fix to consider in the meantime is a function ensureDriverError: (error: unknown) => IAnyDriverError which would be a specialized version of normalizeError which would return any driver error as-is, and would wrap any other error in a new driver error with DriverErrorType.genericError. Then in all the catch blocks anywhere just call ensureDriverError out of the gate, and then deal with it from there. canRetry is on that base interface and if you check errorTypeyou can narrow the type to a type withretryAfterSeconds`.

And other props added for logging can be added in a safe way via addTelemetryProperties rather than tacked onto the any typed error.

markfields avatar Mar 18 '22 17:03 markfields

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

ghost avatar Sep 15 '22 03:09 ghost