fix: Disposal in fluid statics container now disposes instead of closes#26940
fix: Disposal in fluid statics container now disposes instead of closes#26940taylorsw04 wants to merge 2 commits intomicrosoft:mainfrom
Conversation
taylorsw04
requested review from
agarwal-navin,
anthony-murphy,
Copilot and
noencke
| @@ -331,6 +331,7 @@ class FluidContainer<TContainerSchema extends ContainerSchema = ContainerSchema> | |||
| this.acquireExtension = extensionStore.acquireExtension.bind(extensionStore); | |||
| container.on("connected", this.connectedHandler); | |||
| container.on("closed", this.disposedHandler); | |||
There was a problem hiding this comment.
Interesting that there is no "close" event / API on fluid container. I wonder why that's the case.
@ChumpChief Do you know?
There was a problem hiding this comment.
I believe all of this predates any intentionality about the term "close" vs. "dispose", at the time I think we just used both inconsistently throughout the repo.
ChumpChief
left a comment
ChumpChief
left a comment
There was a problem hiding this comment.
Although I'm not a huge fan of the dual closed/disposed states, I think we probably need to either:
- Mirror the underlying Container and expose both here (along with documentation, customer guidance, etc.).
- Treat it like a name alignment - everything becomes "disposed", stop saying "closed" anywhere.
Either way, changing "disposed" to actually mean "disposed" instead of "closed" is a technically-breaking change, and so the safest option would be to align with a major release. But I think an argument could be made that it's not a real threat in practice? Probably raise it with API council to get more diverse opinions.
|
|
||
| public dispose(): void { | ||
| this.container.close(); | ||
| this.container.dispose(); |
There was a problem hiding this comment.
Disposal has different characteristics than close(), which could technically make this a breaking change depending on what the customer is doing post-close today. Have you considered whether this is a concern and/or mitigations or customer guidance if so?
There was a problem hiding this comment.
@ChumpChief is there any documentation anywhere describing the difference? If not, could you explain it? AFAICT, customers of this API have no knowledge of "close" as a concept, although once I understand the difference we can perhaps figure out if they will be impacted by this change.
There was a problem hiding this comment.
Depending on how observable this is, customers may prefer us to fix the memory leak over preserving the existing behavior?
There was a problem hiding this comment.
I believe @kian-thompson did the work to define the difference and might have some good pointers. My basic understanding is that on close, the container remains readable, just not writable. Whereas on dispose, we start to throw for various read operations.
I have no pushback for making one of the two changes I list above here eventually, my point is more about "when" such a change is safe outside of a major release.
There was a problem hiding this comment.
I imagine to preserve previous behavior, you could call both close and dispose
There was a problem hiding this comment.
Since we don't dispose today, wouldn't this start to cause errors thrown if e.g. handles are deref'd post-dispose?
There was a problem hiding this comment.
From when I made this change, old close = new close + dispose. Not sure what new issues would arise, but the behavior should be preserved
| this.container.off("closed", this.disposedHandler); | ||
| this.container.off("disposed", this.disposedHandler); |
There was a problem hiding this comment.
Emitting the same single event for both "closed" and "disposed" may not align well with the underlying system - depending on whether the underlying container closed/disposed the user will observe inconsistent results to further interaction with the FluidContainer, e.g. subsequent calls to retrieve data objects fail or not. Would be good for the event to give a deterministic signal about container state and capabilities.
There was a problem hiding this comment.
Ya, I think I'd like to move to only registering/emitting for disposed, but I wasn't sure how closed works so I'll wait until I do.
| @@ -331,6 +331,7 @@ class FluidContainer<TContainerSchema extends ContainerSchema = ContainerSchema> | |||
| this.acquireExtension = extensionStore.acquireExtension.bind(extensionStore); | |||
| container.on("connected", this.connectedHandler); | |||
| container.on("closed", this.disposedHandler); | |||
There was a problem hiding this comment.
I believe all of this predates any intentionality about the term "close" vs. "dispose", at the time I think we just used both inconsistently throughout the repo.
Description
This PR fixes a bug where the dispose function in the fluid-static FluidContainer called close on the container it wraps rather than dispose (note: dispose calls close). This would result in memory leaks because summarizer timers and any other top-level closures would retain transitive content.
It also ensures that the dispose event fires for both underlying close and dispose events, as the public API makes no distinction between them.