Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#14685 closed defect (wontfix)

dojo.Deferred exhausts stack for long chains

Reported by: skaegi Owned by: Mark Wubben
Priority: undecided Milestone: tbd
Component: Core Version: 1.7.1
Keywords: Cc: ben hockey
Blocked By: Blocking:

Description (last modified by Eugene Lazutkin)

Deferred is using recursion for the chain:

then() -> complete() -> notify() -> {repeat}

This blows the stack when there are a 100 or so elements in the chain in IE, 1000 for FF, and 1700 for Chrome.

Here's a test body to illustrate:

var first = new dojo.Deferred(),
  d = first, i, recurses = 0, max = 5000;
function returnPromise() {
  recurses++;
  return first;
}
for (i = 0; i < max; i++) {
  d = d.then(returnPromise);
}
first.resolve();

Instead of recursion this can be written with a task queue and then queueing the calls to "notify" done in "complete" and "then".

Attachments (1)

test_case_for_parser_failure.patch (4.0 KB) - added by bill 7 years ago.
simplified test case. outer parse finishes befores nested parse

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by bill

Owner: set to Mark Wubben
Status: newassigned

comment:2 Changed 7 years ago by skaegi

Here's an implementation of a "queueTask" method:

allows "notify" to be called without exhausting the stack

	var queueTask = (function() {
		var head, tail, remainingHead, remainingTail, running = false;
		return function (task) {
			if (running) {
				if (!head) {
					head = {task: task, next: null};
					tail = head;
				} else {
					tail.next = {task: task, next: null};
					tail = tail.next;
				}
				return;
			}
			running = true;
			while (task) {
				try {
					task();
				} catch(e) {
					console.error(e);
				}

				if (remainingHead) {
					if (!head) {
						head = remainingHead;
					} else {
						tail.next = remainingHead;
					}
					tail = remainingTail;
				}
				if (head) {
					remainingHead = head.next;
					remainingTail = tail;
					task = head.task;
					head = tail = null;
				} else {
					task = null;
				}
			}
			running = false;
		};
	}());

-- That function has to live at module scope e.g. not in Deferred. I put it after "freeze" and before the Deferred definition.

The calls to "notify()" in "complete" and "then" should become "queueTask(notify);"

comment:3 Changed 7 years ago by ben hockey

Cc: ben hockey added

comment:4 Changed 7 years ago by Mark Wubben

Oh that's an interesting edge case!

I've added a solution and test case to my Deferred code at <https://github.com/promised-io/core/commit/0cb8435abbdaedad351ca32cd4d2dbb1f77bf9db>, the core logic of which should be going into 1.8.

I wonder if this is something we should fix in 1.7 though, more so because seemingly nobody has run into this before? What do you think Bill?

comment:5 Changed 7 years ago by bill

Hmm, well it's more a question of whether we should fix in the 1.x line, in the sense that the new Deferred code won't likely be used much until 2.0 (because changing existing methods to return the new Deferred object would be a slight break in back-compat).

I guess I would fix it in dojo/_base/Deferred.js, but not bother backporting to the 1.7/ branch.

comment:6 Changed 7 years ago by skaegi

This is sort of a messy contribution and probably a bit of a pain to try out -- sorry about that -- really want to be a good contributor here.

What's the best way for you guys to be able to take a better look here? -- do you want me to fork on Github and do a pull request or perhaps create a branch?? Send a unified patch??

We have two additional unit tests for similar cases where instead of a promise the deferred (1)returns a regular value and (2) throws an exception that I could also cleanup and add. FWIW we're (Orion at Eclipse) using this in our 1.6 and 1.7 forks at the moment.

One thing to think about is that although this approach is technically correct there is possibility for breakage where naughty folk have written code like...

somePromise.then(function() {
  someVariable = true;
});
if (someVariable) {...}

e.g. instead of chaining a "then" their is an assumption that if somePromise is previously resolved the success function in the "then" will be called immediately setting "someVariable". Clearly a coding mistake as the check should happen in a then/when block however again something to be aware of though I'm not sure how common that mistake is but still something to consider adding to release notes.

comment:7 Changed 7 years ago by ben hockey

you can attach patches to this ticket

comment:8 in reply to:  6 Changed 7 years ago by bill

Replying to skaegi:

One thing to think about is that although this approach is technically correct there is possibility for breakage where naughty folk have written code like...

somePromise.then(function() {
  someVariable = true;
});
if (someVariable) {...}

Hmm, I'd hate to see that code break, and I don't see why it would, unless somePromise hadn't resolved yet?

comment:9 Changed 7 years ago by Mark Wubben

Hmm, well it's more a question of whether we should fix in the 1.x line, in the sense that the new Deferred code won't likely be used much until 2.0 (because changing existing methods to return the new Deferred object would be a slight break in back-compat).

I guess I would fix it in dojo/_base/Deferred.js, but not bother backporting to the 1.7/ branch.

My plan was to make dojo/_base/Deferred a wrapper around the new dojo/Deferred exposing a backwards-compatible API. Will have to see how feasible that is.

comment:10 Changed 7 years ago by Mark Wubben

Resolution: wontfix
Status: assignedclosed

The new promise code has landed in [28265] and does not have this issue. See also #14615.

Closing this ticket, but let me know if further action is needed in dojo/_base/Deferred.

comment:11 Changed 7 years ago by bill

Resolution: wontfix
Status: closedreopened

The "then() with lots of chaining" test is failing for me on IE8. Not sure if that's testing the new dojo/Deferred module or the old (deprecated) dojo/_base/Deferred module, but either way we shouldn't have a failure in our regression test. The error I get is:

     _AssertFailure: assertTrue('false') failed     ERROR IN: 		 function(t){
			// Test for <http://bugs.dojotoolkit.org/ticket/14685>
			var obj = {};
			var received;
			var promise = this.deferred.promise;
			var count = 0;
			function chain(){
				count++;
				return obj;
			}
			for(var i = 0; i < 5000; i++){
				promise = promise.then(chain);
			}
			this.deferred.resolve();
			promise.then(function(result){ received = result; });
			t.t(received === obj);
			t.is(count, 5000);
		} FAILED test: then() with lots of chaining 4875 ms

(In other words, there's no error message.)

I didn't trace down when the test broke, perhaps when you fixed dojo/Deferred so the parser would run synchronously (when possible).

comment:12 Changed 7 years ago by Mark Wubben

In [28671]:

Actually resolve promise chains without causing stack overflows. Refs #14685 !strict

comment:13 Changed 7 years ago by Mark Wubben

Resolution: wontfix
Status: reopenedclosed

You're right, the change I made for the parser actually made the promise chain overflow the stack. This should fix it.

comment:14 Changed 7 years ago by bill

Resolution: wontfix
Status: closedreopened

Hmm, well it does fix the above failure but now dijit/tests/layout/ContentPane-auto-require.html is getting an error (tested on FF/mac).

comment:15 Changed 7 years ago by Mark Wubben

Yes it is. It's this assertion:

t.is(typeof registry.byId("tb1"), "object", "objected created");

with it being undefined. I don't understand enough about the parser or ContentPane to debug this though.

Changed 7 years ago by bill

simplified test case. outer parse finishes befores nested parse

comment:16 Changed 7 years ago by bill

The above testing patch (which should apply cleanly against [28692]) shows the issue. The problem (at least from my point of view) is that the Deferreds are resolving in an unexpected order.

The parser (after simplification for testing) does this:

// For testing purposes (#14685), create a promise that is already resolved
var deferred = new Deferred(),
        promise = deferred.promise;
deferred.resolve(true);
console.log(id + ": 1. already resolved promise");

var p =
        promise.then(
        function(){
                console.log(id + ": 2. first then()");
                return self.scan(root, options);
        }).then(function(parsedNodes){
                console.log(id + ": 3. second then()");
                return instances = instances.concat(self._instantiate(parsedNodes, mixin, options));
        });
return p;

The _instantiate() call is tricky because it creates a ContentPane which itself makes a (recursive) call of the parser. The recursive call should be completely synchronous, and thus should finish before the original parser call. However, the generated output is:

starting outer parse
outer parse: 1. already resolved promise
outer parse: 2. first then()
outer parse: 3. second then()
ContentPane parse: 1. already resolved promise
ContentPane parse: 2. first then()
outer parse finished        <=== this happens at a strange point
ContentPane parse: 3. second then()
Last edited 7 years ago by bill (previous) (diff)

comment:17 Changed 7 years ago by Eric Wang

I found the API changes by comparing dojo/Deferred with original dojo/_base/Deferred, but no document to show the changes. This also will block other applications.

comment:18 Changed 7 years ago by Mark Wubben

Resolution: fixed
Status: reopenedclosed

In [28719]:

Revert signal queue, recursively resolve promise chains

While nice in theory, using a signal queue breaks expectations around interacting with resolved promises.

Fixes #14685, refs #14615 !strict

comment:19 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

comment:20 Changed 7 years ago by bill

Resolution: wontfix
Status: reopenedclosed

comment:21 Changed 7 years ago by Eugene Lazutkin

Description: modified (diff)
Note: See TracTickets for help on using tickets.