Determine the behavior when calling unobserve/disconnect method before the promise returned by observe method is settled
wangw-1991 opened this issue · 20 comments
The following codes will cause the promise never resolve or reject in current implementation. It is more reasonable to reject these promises instead and the spec should give more clarity about this.
let promise = observer.observe('cpu');
observer.disconnect();
More generally speaking, PressureObserver.observe() returns a new Promise every time it is invoked, and PressureObserver.{disconnect,unobserve}() may be called while observe() is in the "in parallel" steps. It is not clear what should be done in this case as they should probably influence what happens to the unfulfilled promises.
Maybe this would work:
- store pending observe promises
- disconnect: reject all pending observe promises, but make sure that everything has been removed from "active observer list" - that should be fine with current spec text
- unobserve: iterate over the pending promises and reject the ones matching the source. Remove from "active observer list" - might have been added, might not
Biggest issue I see if the "global task" as that will run when after JS has completed, including the sync disconnect/unobserve, so it will readd to the active observer list and activate the observation, so that code would probably have to check whether it's promise has been removed from the pending observer promises list and if so, don't do any work
Given it looks like we will remove the async permission stuff, this becomes a non-issue. But we should keep using promises in observe() so that we can add it back.
Given the current Blink implementation, I think this will still be an issue even if the permission bits are removed, as there are async steps involved in checking if source is a supported source type and when activating data delivery.
It could be possible to stop distinguishing between a successful async connection to the platform side (and maybe even drop the NotSupportedError exception related to checking for supported source types), but one would still have a window where the async connection is being established internally but disconnect/unobserve can still be called.
For the record, at least Web Bluetooth and WebHID also need to deal with this:
Given that we use an attribute for supported source types, we have not made that an async querying function, so we could do this check sync and it should not be an issue.
If we want to make that async in the future, it does become a problem yes.
If that is indeed async, how is the attribute implemented today then? Also the current activation was happening on the main tread (in the global task) so the promise resolve wasn't waiting on that, you just get the events when they are delivered
one would still have a window where the async connection is being established internally but disconnect/unobserve can still be called.
We haven't really spec'ed how that happens, and in the spec that also happens in the global task (main thread) so if we want to do that, we need to find out how to make these changes
@rakuco I did a PR to remove the permissions integration, but I can change it to keep the global task and instead do the activation in parallel. Then afterwards we could do the pendingPromises. If you think that is the way to go, tell me and I will prepare the PRs
If that is indeed async, how is the attribute implemented today then?
The supportedSources attribute just synchronously returns a hardcoded list at the moment. The observe() implementation queries the platform collector to know what source types are actually supported (e.g. calling observer('cpu') on Android would reject with a NotSupportedError).
Reading the spec again, it looks like supportedSources should also return what the platform collector supports though, which means the list can only be built asynchronously and there's a period where it won't have data to return.
Am I getting this correctly?
PressureSourcecontains all source types the spec recognizes.- What source types are actually supported depends on a combination of what the user agent implements and what the OS/hardware supports, which can only be determined asynchronously.
- The result above is used by both the
supportedSourcesattribute and the check inobserve(). The latter can be done asynchronously, but the former cannot.
- The result above is used by both the
@arskama mentioned that the attribute getter returns only values supported by the spec and not necessarily the underlying hardware - I believe that is how it worked in some other specs, we can consider whether to change that or not.
@arskama mentioned that the attribute getter returns only values supported by the spec and not necessarily the underlying hardware
Values supported by the spec or by the implementation? If it's the former, then PressureSource suffices; if it's the latter, then it's either what the platform supports (which you only know asynchronously) or it's a third state that means "supported by the implementation but not necessarily by the platform" (in which case supportedSources does not necessarily return the same values as those used in the observe() check).
Basically what the browser supports on at least one of its platforms
Btw looking at those WebHID pending promises, I don't ever see them remove promises as they resolve. Looks like a mistake, or am I missing something?
https://wicg.github.io/compute-pressure/#platform-primitives
As written in the section mentioned above. The platform collector is the SW proxy to access the HW,which means that it can be all synchronously requested.
The question remaining is: How do we inform the user on the case where the HW doesn't support the platform collector requests? If observe is synchronous, there is no way to do it.
@Elchi3 I was wondering if you have any input on what supportedSources should expose. Right now it exposes the sources supported by the user agent (browser) but not necessarily supported on a platform (say Android) or by the hardware.
Should we change that and make it an async getter, or add a similar method with that behavior in addition to the attribute?
@Elchi3 I was wondering if you have any input on what supportedSources should expose. Right now it exposes the sources supported by the user agent (browser) but not necessarily supported on a platform (say Android) or by the hardware.
Good to know, I need to clarify this in the docs. So users will only find out if it is really supported in the current environment when calling observe()? In that case it seems supportedSources is merely a hint then?
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceObserver/supportedEntryTypes_static is the same thing for PerformanceObserver but I guess given the Perf APIs aren't depending on specific hardware, it is fine as a static property there.
Should we change that and make it an async getter, or add a similar method with that behavior in addition to the attribute?
Not sure if there is a standard way for it on the web platform but a method is used elsewhere (especially when it has to do with hardware support, see WebXR and WebGL).
https://developer.mozilla.org/en-US/docs/Web/API/CSS/supports_static
https://developer.mozilla.org/en-US/docs/Web/API/XRSystem/isSessionSupported
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getSupportedExtensions
https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getSupportedConstraints
If we decide to do this, we can remove the existing API for now and if we don't make it, we can add this new method after shipping
@tomayac you feedback is also welcome
Before M125 release, I don't see it happening.
So if we want to provide the platform collector source list, then it s maybe better to remove it now and add it later when we have an implementation.