implement IDisposable in IDocumentStorageService

[znewton]

First PR to address #5980. After this is checked in and released, I have changes staged locally to consume this type change across client packages.

[anthony-murphy]

Suggested change

	export interface IDocumentStorageService extends IDisposable {
	export interface IDocumentStorageService extends Partial<IDisposable> {

we should avoid breaking changes. can we start with making in optional, so implementations don't break

[anthony-murphy]

also is this totally necessary on the interface? is it possible to only use on the implementation? who will be expected to call this?

[znewton]

Currently it is only used on the implementation, so yes, it's possible. However, doing so can restrict the "wrapping" order of DocumentStorageServices. The primary example, and reason that the issue for this was opened, is RetriableDocumentStorageService, which implements IDisposable. There was a bug in PrefetchDocumentStorageService, which was wrapped by RetriableDocumentStorageService. @vladsud recommended simply changing the wrapping order to address it. Without this type change, it's messy for storage services that wrap others to check if the internal storage service is disposable and should be disposed.

Let me know if I misunderstood the question, but it seems there has been enough other people say "IDocumentStorageService should just always extend IDisposable" (@vladsud, you, and @ChumpChief) that this seems like an OK change, even if not totally necessary.

[vladsud]

I'd say - yes they should. But that might still require staged rollout / injecting some delays. Basically there is no easy way to tie version of protocol / definitions to versions and expectations of packages, so declaring it required allows for loader / runtime layer to take advantage of it while using old driver version. So likely we want to add implementations, ship them, and then after some time make interface requirement non-optional. It sucks, but such is life of supporting back compat

[znewton]

Thanks for all the input, Anthony and Vlad. I've made the interface change optional, which makes it back/forward compatible.

[ChumpChief]

My main question would be the same as Tony's, "who will be expected to call this?"

Is it OK for anyone with access to an IDocumentStorageService to call dispose()? I would guess probably not? So if there's some way to restrict access to whoever is the manager of its lifetime would be good.

[anthony-murphy]

i think leaving it partial it probably the right answer. this allows anyone that want to manage the lifetime to expose a wrapper without dispose to consumers, but makes it easy for implementors to know where to clean up if they need to

[znewton]

I'm maybe not familiar enough with the runtime and loader space to best answer that question 😕 . I would think it's not too different from today though where the top level DocumentStorageService is a RetriableDocumentStorageService which extends IDisposable already. My intent here would be to make it easier for the top-level DocumentStorageService to be something other than RetriableDocumentStorageService, but still make it possible to dispose of that service and its internal services.