sysapps / device-capabilities

API to retrieve the status of the device capabilities supported and system information from the underlying device.
2 stars 7 forks source link

Race condition when getting storage information #14

Closed tmpsantos closed 10 years ago

tmpsantos commented 10 years ago

There is a race condition between calling getStorageInfo() and subscribing to the onattach event.

At the time you get the results of getStorageInfo(), a device might be attached, but you are not subscribed to the onattach event.

So the only reliable way of getting the list of storages would be calling getStorageInfo(), subscribing to the onattach event and them calling getStorageInfo() again.

Maybe the best way of fixing this would be making the DeviceCapabilities an EventTarget and not the SystemStorage.

anssiko commented 10 years ago

+1 to the fix proposed by @tmpsantos This is the updated WebIDL, changed the on* handler names a bit for clarity:

interface DeviceCapabilities : EventTarget {
    Promise getCPUInfo ();
    Promise getMemoryInfo ();
    Promise getStorageInfo ();
    Promise getDisplayInfo ();
    Promise getAVCodecs ();
    attribute EventHandler  onstorageattach;     // was onattach
    attribute EventHandler  onstoragedetach;     // was ondetach
    attribute EventHandler  ondisplayconnect;    // was onconnect
    attribute EventHandler  ondisplaydisconnect; // was ondisconnect
};

After the above change, we should drop the corresponding on* handlers from the SystemStorage and SystemDisplay interfaces.

Also, should clarify the flow of querying storages addressing @tmpsantos concern above. An usage example for SystemStorage might help.

@hmin are you able to update the specification accordingly?

hmin commented 10 years ago

@tmpsantos @anssiko Thanks for you raise this issue. I will update the spec.

tmpsantos commented 10 years ago

@hmin Thanks for fixing so quickly! Just a layout nit, the "EventHandlers" are not properly aligned in the document.

hmin commented 10 years ago

@tmpsantos 'readonly' is added. Thanks.

anssiko commented 10 years ago

@tmpsantos Thanks for reporting the spec bug in the first place!

Re the layout issue. The ReSpec tool used to produce the specification takes care of indentation within the WebIDL block, and the behaviour cannot be altered without patching the tool itself. You can consider this a feature of the ReSpec tool :-)