feat(Container): Add IContainer.closedWithError to access the error that closed or disposed the container #23364
Conversation
markfields
requested review from
Copilot and
kian-thompson
and removed request for
Copilot
github-actions
bot
added
area: loader
markfields
requested review from
a team,
pragya91,
jatgarg,
tyler-cai-microsoft,
rajatch-ff and
MarioJGMsoft
and removed request for
a team
| category: !this.closed && error !== undefined ? "error" : "generic", | ||
| // Use error category if there's an error, unless we already closed with an error (i.e. _criticalError is set) | ||
| category: | ||
| error !== undefined && this._criticalError === undefined ? "error" : "generic", |
There was a problem hiding this comment.
I'm thinking that the closed state is a legit (non-error) state for a container to be in, if it was closed with no error (consider the fact that we wouldn't log that "ContainerClosed" event as an error). Then if it's disposed with an error, that'd be the point where we'd want to see the error log.
i.e. if you close (no error) then dispose (with error), before you would get no error logs. Now you do get an error log.
| // When disposing with an error... | ||
| // - if we closed with an error, _criticalError doesn't expose the dispose error. | ||
| // - if we closed without an error, _criticalError doesn't distinguish whether this error came from close or dispose. | ||
| this._criticalError ??= error; |
There was a problem hiding this comment.
similarly to pushing the other set of this variable, i also wonder if this should be done higher up. i also wonder if we should only set if is undefined, and the container is not closed. this would make some of the caveats above go away, as this would always represent the error the container is closed with.
the only difference is that if the container is previously closed without error, this closeWithError will never be set, which makes sense to me.
There was a problem hiding this comment.
only set if is undefined, and the container is not closed
I thought about that, but I think if dispose can take an error, we should be able to access it this way.
(This is also part of why I didn't name it with the term close at first)
There was a problem hiding this comment.
i think i would be willing to lose the dispose error when closed without error, the case seems uncommon, and removing it keeps everything consistent.
in fact, inspecting the container object after dispose isn't really supported, as dispose should mean all usage is done, and we don't guarantee anything about the state after that.
There was a problem hiding this comment.
Chatted offline. In conclusion, I'm going to switch it as suggested. Key reasoning:
I'm adding this to fill a particular gap, in the Container Load flow. And this corner case is not relevant there. So if someone is very concerned about getting the dispose error, they will have the event to listen to. And... why are you using a container after handling the dispose event anyway
|
@anthony-murphy -- I just pushed, this is ready for final review. Thanks for the help on this one! |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
anthony-murphy
changed the title
| }, | ||
| ); | ||
|
|
||
| itExpects( |
There was a problem hiding this comment.
nit. end to end test are costly to run and maintain, did you consider a unit test, or a local-server test?
There was a problem hiding this comment.
Great point. I started by trying to unit test... there are NO existing unit tests for Container class! So decided not to be the tip of that spear. Local-server only is a good idea though. Probably most of this file should be local-server only.
There was a problem hiding this comment.
Turns out all these e2e tests only run against local, so that's good. We could move them to local-server-tests though at some point
Description
Fixes AB#13773
Two cases I know of where one would want to be able to inspect a closed container to see what the error was:
Container.loadis returning it (interleaved microtasks), so the "closed" event never fires and the error is not thrown.UsageError, and the caller didn't expect the container to be closed and wants to know why (for debugging or telemetry purposes)Breaking Changes
Nope, it's adding an optional property to an interface.
Reviewer Guidance
@microsoft/fluid-cr-api -- Please provide feedback / suggestions on the naming and semantics of this new API. I tried to keep it simple, at the cost of some ambiguity in corner cases (e.g. when you close and dispose with different errors).