Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16389 closed defect (fixed)

domReady() method doesn't handle recursive or concurrent calls well

Reported by: bill Owned by: bill
Priority: undecided Milestone: 1.9
Component: Core Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

dojo/ready will detect if it's called recursively, and then queue the new callback to execute after the current callback finishes. However, domReady() doesn't do this. It's also an issue if domReady() is called from two places concurrently.

Note that this ticket is referring to dojo/domReady used as a function, rather than as a plugin.

This is the main cause of the problem listed in #16386 but I figured it deserved it's own ticket.

Change History (6)

comment:1 Changed 7 years ago by bill

Milestone: tbd1.9
Owner: set to bill
Status: newassigned

comment:2 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [30057]:

Make sure that domReady(), when used as a function rather than a plugin, executes registered callbacks sequentially. This is how ready() is supposed to work, so it makes sense for domReady() to follow suit.

Plus, it's needed to make ready() work as advertised, since ready() calls domReady() internally. Note that it's still dodgy that there are two queues, one in dojo/ready and one in dojo/domReady. However, since dojo/ready is essentially deprecated, that's probably not worth fixing.

This fix is mainly a recursive guard, i.e. if f1() calls domReady(f2), f2() will be delayed until f1() completes. However, it also addresses a concurrency issue:

In Editor_mouse.html on chrome I noticed that DOH started running the tests while test_Editor.s dojo.ready() callback was still executing. It was mainly because the ready(999, ...) call from DOH occurred while the test_Editor.html dojo.ready() callback was running. Normally browsers don't run multiple threads but I guess this is an exception since DOH and test_Editor.html are in different frames.

Also added a try/catch around each domReady() callback so that a failure in one callback won't affect other code, and won't get the guard variable stuck.

Fixes #16389, refs #16386 !strict.

comment:3 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

Argh, that broke dojo/tests/parrser/parseOnLoadDeclarativeRequire.html.

Version 0, edited 7 years ago by bill (next)

comment:4 Changed 7 years ago by bill

Another good test is dijit/tests/_BidiSupport/layout/AccordionContainer.html. The ready() call inside of dojo/parser.js triggers the parser to auto-load some modules, indirectly including _CssStateMixin, and then _CssStateMixin calls domReady().

The second time AccordionContainer.html is run, without clearing the cache, all of this happens synchronously, which uncovered another bug on IE8 where the callback in the domReady() call in _CssStateMixin wasn't getting run.

comment:5 Changed 7 years ago by bill

Resolution: fixed
Status: reopenedclosed

In [30282]:

Second try to get dojo/ready and dojo/domReady! to play nice together. dojo/ready still needs to yield to any tasks registered via domReady(), but callbacks in the dojo/ready queue cannot be removed until they are ready to be run. Fixes #16389 !strict. Needed to add some hooks into the dojo/domReady code so that dojo/ready can yield to tasks in that queue, but they can be removed for 2.0.

comment:6 Changed 7 years ago by bill

In [30322]:

Get dojo/ready working on a non-browser environment again, so the builder works. I'm unclear why dojo/ready is getting called at all in that case though. Refs #16389, fixes #16557 !strict

Note: See TracTickets for help on using tickets.