[CLOSED] Clean up Live Development agent loading

[core-ai-bot]

Issue by iwehrman
Thursday Mar 21, 2013 at 18:39 GMT
Originally opened as adobe/brackets#3203

---

Currently, Live Development agents resolve their load() promises before remote services they rely on have been completely enabled. For example, NetworkAgent.load() calls Inspector.Network.enable() but does not wait for this asynchronous call to complete before returning. This pull request delays completion of these load calls until the necessary asynchronous calls have successfully completed.

I'm not aware of any problems caused by the current behavior, but fewer race conditions seems better than more race conditions. Startup races like this also tend to manifest themselves during automated testing.

---

iwehrman included the following code: https://github.com/adobe/brackets/pull/3203/commits

[core-ai-bot]

Comment by redmunds
Thursday Mar 21, 2013 at 22:48 GMT

---

In loadAgents(), agent names are synchronously added to the _loadedAgentNames array after calling agents[name].load(). They should only be added if the call to load() is resolved.

[core-ai-bot]

Comment by redmunds
Thursday Mar 21, 2013 at 23:33 GMT

---

I don't think we want to take this for Sprint 22. Those are all of the comments I had.

[core-ai-bot]

Comment by jasonsanjose
Tuesday Apr 09, 2013 at 04:32 GMT

---

Looks like this will miss the Sprint 23 boat. It's probably too late to take this change.@iwehrman, can you address@redmunds' comments for sprint 24? Thanks.

[core-ai-bot]

Comment by iwehrman
Tuesday Apr 09, 2013 at 19:00 GMT

---

I think the requested changes to loadAgents() are unnecessary and/or out of scope here.

It shouldn't matter that the addition to _loadedAgentNames is synchronous because _loadedAgentNames is only used by unload(). Calls to unload() should happen after the promises returned by load() have resolved---though, see the caveat below---but even if they don't this just means that the agent names will be added sooner rather than later. And the agents should be unloaded regardless of whether they resolve successfully or unsuccessfully because, in case the load fails, the call to unload() is used to clean up after the failure. (We could alternatively change all the agents so they always call unload() on themselves if they fail, but I'm not sure that would be any better.)

Unfortunately, there is yet another problematic race between loadAgents() and unloadAgents that this patch doesn't address: _onDisconnect, and hence unloadAgents(), can be fired before loadAgents() has finished, or even started. Note that this is a problem with or without this patch. Ideally _onDisconnect() would be changed to block until all the agents have finished loading one way or another. But it's not completely obvious how to do this, and I don't have time to make that change right now. A new bug should be filed for this race.

[core-ai-bot]

Comment by redmunds
Tuesday Apr 09, 2013 at 19:13 GMT

---

I agree that the issue I have with the _loadedAgentNames list does not seem to cause any known problem. It just seems to imply that it contains a list of successfully loaded agents and someone may try to use that for something in the future. If it simply contains the same list as _enabledAgentNames, then it's not needed, right? If it is needed, then it should be accurate. Or maybe the name should be changed to describe what's actually in the list?

[core-ai-bot]

Comment by iwehrman
Tuesday Apr 09, 2013 at 21:10 GMT

---

According to the text of loadAgents(), I don't believe _loadedAgentsNames contains the same list as _enabledAgentNames. Yes, it might be safe to eliminate this list entirely and unload all possible agents in unloadAgents(). But the race I mentioned above would still exist and still be problematic.

I agree that this function sucks, but I don't immediately know how/have time to fix it the right way. I think a new bug is appropriate here. The changes in this patch don't introduce any new problems with this function (as far as I can tell) because some agents are already loading asynchronously.

[core-ai-bot]

Comment by redmunds
Tuesday Apr 09, 2013 at 22:10 GMT

---

I opened a new issue for the _loadedAgentNames cleanup: adobe/brackets#3390

[core-ai-bot]

Comment by redmunds
Monday Apr 15, 2013 at 17:39 GMT

---

@jasonsanjose It sounds like the changes in this pull requests are related to the Live Dev research you are doing. I was hoping to merge this soon, so let me know if I should hold off.

[core-ai-bot]

Comment by jasonsanjose
Monday Apr 15, 2013 at 17:46 GMT

---

Thanks for the heads up. I'll let you know soon.

[core-ai-bot]

Comment by jasonsanjose
Wednesday Apr 17, 2013 at 01:08 GMT

---

Closing this pull request to manually reconcile with #3442.