Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

also send notify/progress across connection #20

Merged
merged 1 commit into from
Jul 3, 2013
Merged

also send notify/progress across connection #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 34 additions & 7 deletions q-connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,12 @@ function Connection(connection, local, options) {
var receivers = {
"resolve": function (message) {
if (locals.has(message.to)) {
resolveLocal(message.to, decode(message.resolution));
resolveLocal(message.to, 'resolve', decode(message.resolution));
}
},
"notify": function (message) {
if (locals.has(message.to)) {
resolveLocal(message.to, 'notify', decode(message.resolution));
}
},
// a "send" message forwards messages from a remote
Expand All @@ -65,6 +70,7 @@ function Connection(connection, local, options) {
// which will return a response promise
var local = locals.get(message.to).promise;
var response = Q.dispatch(local, message.op, decode(message.args));
var envelope;

// connect the local response promise with the
// remote response promise:
Expand All @@ -81,7 +87,7 @@ function Connection(connection, local, options) {
resolution = {"!": null};
}
}
var envelope = JSON.stringify({
envelope = JSON.stringify({
"type": "resolve",
"to": message.from,
"resolution": resolution
Expand All @@ -103,6 +109,27 @@ function Connection(connection, local, options) {
"resolution": {"!": reason}
})
connection.put(envelope);
}, function (progress) {
try {
progress = encode(progress);
envelope = JSON.stringify({
"type": "notify",
"to": message.from,
"resolution": progress
});
} catch (exception) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure we want to reject the remote promise if the local promise emits a non-serializable progress value. On the other hand, I’m not sure what’s proper in any case. In Q proper, if a progress handler throws an exception, we let the app crash https://github.com/kriskowal/q/blob/master/q.js#L727-L729

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I am not mistaken, I should remove the try/catch block then?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s leave it as-is for the moment. I think we should bring in other stake holders, particularly @domenic, to think about a more satisfying solution in general. Perhaps rejecting the target promise is the best way to communicate that the progress handler had trouble coping.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far I think @ForbesLindesay's solution in promises-aplus/progress-spec#3 (comment) is the most satisfying. I'm not sure exactly what's going on in this pull request, but it seems possibly related...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic definitely related. We should make that work in Q and swing back to this issue.

try {
progress = {"!": encode(exception)};
} catch (exception) {
progress = {"!": null};
}
envelope = JSON.stringify({
"type": "resolve",
"to": message.from,
"resolution": progress
});
}
connection.put(envelope);
})
.done();

Expand All @@ -123,9 +150,9 @@ function Connection(connection, local, options) {

// a utility for resolving the local promise
// for a given identifier.
function resolveLocal(id, value) {
_debug('resolve:', "L" + JSON.stringify(id), JSON.stringify(value), typeof value);
locals.get(id).resolve(value);
function resolveLocal(id, op, value) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that we’re not getting much utility from this utility function. The resolve in resolveLocal certainly no longer applies. Perhaps we should inline this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are currently 4 instance of use of this util function: line 56, 62, 191 and 261. I am for making it inline for line 56 and 62, but I have insufficient knowledge on other two instances. If you can suggest a solution, I will make the changes

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll take care of it in a follow-up. They are all the same, really. The other cases are for encoding references to local promises in message arguments.

_debug(op + ':', "L" + JSON.stringify(id), JSON.stringify(value), typeof value);
locals.get(id)[op](value);
}

// makes a promise that will send all of its events to a
Expand Down Expand Up @@ -161,7 +188,7 @@ function Connection(connection, local, options) {
} if (Q.isPromise(object) || typeof object === "function") {
var id = makeId();
makeLocal(id);
resolveLocal(id, object);
resolveLocal(id, 'resolve', object);
return {"@": id, "type": typeof object};
} else if (object instanceof Error) {
return {
Expand Down Expand Up @@ -231,7 +258,7 @@ function Connection(connection, local, options) {
// the root object is an empty-string by convention.
// All other identifiers are numbers.
makeLocal(rootId);
resolveLocal(rootId, local);
resolveLocal(rootId, 'resolve', local);
return makeRemote(rootId);

}
Expand Down
25 changes: 25 additions & 0 deletions spec/q-connection-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,31 @@ describe("remote promises that fulfill to functions", function () {
});
});

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test case.

describe("remote promises that notify progress", function () {
it("should trigger local progress handler", function () {
var peers = makePeers({
resolveAfterNotify: function (times) {
var deferred = Q.defer();
var count = 0;
setTimeout(function () {
while (count++ < times) {
deferred.notify('Notify' + count + ' time');
}
deferred.resolve('Resolving');
}, 0);
return deferred.promise;
}
});

var notifyCount = 0;
return peers.remote.invoke('resolveAfterNotify', 3).progress(function(p) {
notifyCount++;
}).then(function (message) {
expect(notifyCount).toBe(3);
});
});
});

describe("rejection", function () {
it("should become local functions", function () {
var peers = makePeers({
Expand Down