Skip to content

Commit

Permalink
JS Requester handles disposal-and-exception-while-sending
Browse files Browse the repository at this point in the history
Improve the JS Requester class to gracefully handle this edge case:
the send() call triggers not only an exception, but also the disposal
of the message channel. The bug was that we were trying to reject the
same request twice (first during disposal, second while handling the
exception).

This is follow-up to the crash handling issue fixed in #823.
  • Loading branch information
emaxx-google committed Sep 14, 2023
1 parent ecb0da8 commit 6bdacfb
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 8 deletions.
49 changes: 43 additions & 6 deletions common/js/src/requesting/requester-unittest.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,18 @@ class PromiseTracker {
}
}

/**
* @param {!goog.testing.messaging.MockMessageChannel} mockMessageChannel
*/
function disposeOfMockMessageChannel(mockMessageChannel) {
mockMessageChannel.dispose();
// Hack: call the protected disposeInternal() method in order to trigger the
// disposal notifications; the MockMessageChannel.dispose() doesn't call
// them.
mockMessageChannel[goog.reflect.objectProperty(
'disposeInternal', mockMessageChannel)]();
}

goog.exportSymbol('testRequester', function() {
const REQUESTER_NAME = 'test-requester';
const REQUEST_MESSAGE_TYPE = getRequestMessageType(REQUESTER_NAME);
Expand Down Expand Up @@ -133,12 +145,7 @@ goog.exportSymbol('testRequester', function() {
// After that, dispose of the message channel. The promise for request #0 gets
// rejected.
const request0CompletionPromise = request1CompletionPromise.then(function() {
mockMessageChannel.dispose();
// Hack: call the protected disposeInternal() method in order to trigger the
// disposal notifications; the MockMessageChannel.dispose() doesn't call
// them.
mockMessageChannel[goog.reflect.objectProperty(
'disposeInternal', mockMessageChannel)]();
disposeOfMockMessageChannel(mockMessageChannel);
return request0Promise.then(null, function(rejectionError) {
assert(request0PromiseTracker.isResolved);
assert(request1PromiseTracker.isResolved);
Expand Down Expand Up @@ -169,6 +176,8 @@ goog.exportSymbol('testRequester_disposed', function() {
}, () => {});
});

// Test that an exception happening while sending the request message results in
// a rejected response promise.
goog.exportSymbol('testRequester_exceptionWhileSending', async function() {
const REQUESTER_NAME = 'test-requester';
const SEND_ERROR_MESSAGE = 'fake send error';
Expand All @@ -192,4 +201,32 @@ goog.exportSymbol('testRequester_exceptionWhileSending', async function() {
}
fail('Message posting unexpectedly succeeded');
});

// Test that if sending the request message results in immediate disposal and an
// exception, the response promise is rejected correctly.
goog.exportSymbol(
'testRequester_exceptionAndDisposalWhileSending', async function() {
const REQUESTER_NAME = 'test-requester';
const SEND_ERROR_MESSAGE = 'fake send error';

const mockControl = new goog.testing.MockControl();
const mockMessageChannel =
new goog.testing.messaging.MockMessageChannel(mockControl);
/** @type {?} */ mockMessageChannel.send;
// Set up mock that throws an exception when a message is sent.
mockMessageChannel.send(ignoreArgument, ignoreArgument).$does(() => {
disposeOfMockMessageChannel(mockMessageChannel);
throw new Error(SEND_ERROR_MESSAGE);
});
mockMessageChannel.send.$replay();
const requester = new Requester(REQUESTER_NAME, mockMessageChannel);

try {
await requester.postRequest({});
} catch (e) {
// This is the expected branch.
return;
}
fail('Message posting unexpectedly succeeded');
});
}); // goog.scope
7 changes: 5 additions & 2 deletions common/js/src/requesting/requester.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,11 @@ GSC.Requester = class extends goog.Disposable {
this.messageChannel_.send(serviceName, messageData);
} catch (e) {
// If sending the message triggered an exception, use it to populate the
// response.
this.rejectRequest_(requestId, e);
// response. However, this is already done if the channel got immediately
// disposed of, since `disposeInternal()` cancels all ongoing requests.
if (!this.isDisposed()) {
this.rejectRequest_(requestId, e);
}
}

return promiseResolver.promise;
Expand Down

0 comments on commit 6bdacfb

Please sign in to comment.