Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2832303002:
Fifo order should be preserved for messages on associated interfaces. (Closed)
|
Created:
3 years, 8 months ago by wangjimmy Modified:
3 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFifo order should be preserved for messages on associated interfaces.
Cache the current message and pause processing the incoming messages if an endpoint does not have client attached yet.
Resume processing the incoming messages when the endpoint client is attached to that endpoint.
// Expect arrival of message with value 456, then message with value 123.
integerSender1.send(456) // Binding side not bound yet so no endpoint client.
integerSender0.send(123)
BUG=695635
Review-Url: https://codereview.chromium.org/2832303002
Cr-Commit-Position: refs/heads/master@{#467427}
Committed: https://chromium.googlesource.com/chromium/src/+/d65d570dafc96e2f12abe682c7148a350a94dc52
Patch Set 1 #Patch Set 2 : Formatting #Patch Set 3 : Add string sender and use that instead of integer sender twice. #
Total comments: 12
Patch Set 4 : Address codereview #
Total comments: 9
Patch Set 5 : Check target endpoint's client still exists. Remove associated_binding commited by mistake. #Patch Set 6 : Check cacheMessageData is set then get cachedMessage interface id #
Total comments: 4
Patch Set 7 : If target endpoint closed, discard cache message and connector resume processing incoming messages. #
Total comments: 6
Patch Set 8 : Remove timer from closeEndpointHandle. Clean up associated_interface_ptr.html test. #
Messages
Total messages: 45 (31 generated)
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
wangjimmy@chromium.org changed reviewers: + yzshen@chromium.org
Hi Yuzhu, PTAL at all. Thank you
wangjimmy@chromium.org changed reviewers: + rockot@chromium.org
Hi Ken, PTAL at all. Thank you
Description was changed from ========== Fifo order should be preserved for messages on associated interfaces. Cache the current message and pause processing the incoming messages if an endpoint does not have client attached yet. Resume processing the incoming messages when the endpoint client is attached to that endpoint. // Expect arrival of message with value 456, then message with value 123. integerSender1.send(456) // Binding side not bound. No endpoint client. integerSender0.send(123) BUG=695635 ========== to ========== Fifo order should be preserved for messages on associated interfaces. Cache the current message and pause processing the incoming messages if an endpoint does not have client attached yet. Resume processing the incoming messages when the endpoint client is attached to that endpoint. // Expect arrival of message with value 456, then message with value 123. integerSender1.send(456) // Binding side not bound yet so no endpoint client. integerSender0.send(123) BUG=695635 ==========
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2832303002/diff/40001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/40001/mojo/public/js/router.j... mojo/public/js/router.js:75: this.cachedMessageData = null; nit: Please consider commenting on the meaning of this field. https://codereview.chromium.org/2832303002/diff/40001/mojo/public/js/router.j... mojo/public/js/router.js:138: var ok = interfaceEndpointClient.handleIncomingMessage( We are not supposed to call it synchronously. Because this will re-enter users' code. Imagine the user calls: var associated_binding = new AssociatedBindings(..., new FooImpl(), ...); If we call into "handleIncomingMessage" synchronously, that means FooImpl's methods may be called before newAssociatedBindings() returns. https://codereview.chromium.org/2832303002/diff/40001/mojo/public/js/router.j... mojo/public/js/router.js:218: return false; I think we should return "true" to mean "the message processing succeeded"? I understand this return value is not currently used by the Connector. https://codereview.chromium.org/2832303002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html (right): https://codereview.chromium.org/2832303002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html:126: integerSender0.ptr.bind(integerSenderPtrInfo0); nit: It would be more concise if you directly pass |integerSenderPtrInfo0| into the constructor above. (here and below) https://codereview.chromium.org/2832303002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html:254: var value = await new Promise(function(resolve, reject) { I feel that this test is unnecessarily complex. If we want to test FIFO-ness, we could set up two (or more) associated interfaces on a pipe, say, impl0 and impl1. And let them record the calls they received into a shared array. And at the end we examine the values in this array and check that the elements are in order. That seems much easier to understand (and extensible). WDYT? https://codereview.chromium.org/2832303002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html:301: }, `cache current message and pause processing incoming messages if endpoint int: I think you could convert this long description into a comment. And make this description shorter? It will be more consistent with other test cases.
https://codereview.chromium.org/2832303002/diff/40001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/40001/mojo/public/js/router.j... mojo/public/js/router.js:75: this.cachedMessageData = null; On 2017/04/24 17:30:46, yzshen1 wrote: > nit: Please consider commenting on the meaning of this field. Done. https://codereview.chromium.org/2832303002/diff/40001/mojo/public/js/router.j... mojo/public/js/router.js:138: var ok = interfaceEndpointClient.handleIncomingMessage( On 2017/04/24 17:30:45, yzshen1 wrote: > We are not supposed to call it synchronously. Because this will re-enter users' > code. > > Imagine the user calls: > > var associated_binding = new AssociatedBindings(..., new FooImpl(), ...); > > If we call into "handleIncomingMessage" synchronously, that means FooImpl's > methods may be called before newAssociatedBindings() returns. Done. https://codereview.chromium.org/2832303002/diff/40001/mojo/public/js/router.j... mojo/public/js/router.js:218: return false; On 2017/04/24 17:30:45, yzshen1 wrote: > I think we should return "true" to mean "the message processing succeeded"? > I understand this return value is not currently used by the Connector. Done. https://codereview.chromium.org/2832303002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html (right): https://codereview.chromium.org/2832303002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html:126: integerSender0.ptr.bind(integerSenderPtrInfo0); On 2017/04/24 17:30:46, yzshen1 wrote: > nit: It would be more concise if you directly pass |integerSenderPtrInfo0| into > the constructor above. (here and below) Done. https://codereview.chromium.org/2832303002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html:254: var value = await new Promise(function(resolve, reject) { On 2017/04/24 17:30:46, yzshen1 wrote: > I feel that this test is unnecessarily complex. > > If we want to test FIFO-ness, we could set up two (or more) associated > interfaces on a pipe, say, impl0 and impl1. And let them record the calls they > received into a shared array. > > And at the end we examine the values in this array and check that the elements > are in order. > > That seems much easier to understand (and extensible). > WDYT? Done. Modified mojom to not be passing in callback type (resolve or reject). https://codereview.chromium.org/2832303002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html:301: }, `cache current message and pause processing incoming messages if endpoint On 2017/04/24 17:30:46, yzshen1 wrote: > int: I think you could convert this long description into a comment. And make > this description shorter? It will be more consistent with other test cases. Done.
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2832303002/diff/60001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/60001/mojo/public/js/router.j... mojo/public/js/router.js:140: timer.createOneShot(0, (function() { TODO(wangjimmy): Check endpoint client still exists here.
https://codereview.chromium.org/2832303002/diff/60001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/60001/mojo/public/js/router.j... mojo/public/js/router.js:140: timer.createOneShot(0, (function() { On 2017/04/24 19:04:33, wangjimmy wrote: > TODO(wangjimmy): Check endpoint client still exists here. I hope this could be addressed in this CL, which shouldn't be too difficult. When the timer fires, you need to use the interface ID from the cachedMessageData to loop up the endpoints map, and see whether it has got a client attached, if yes, you can directly called into client. https://codereview.chromium.org/2832303002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/mojo/associated_binding.html (right): https://codereview.chromium.org/2832303002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_binding.html:34: var integerSenderBinding = new associatedBindings.AssociatedBinding( As a local variable, this could be garbage-collected. https://codereview.chromium.org/2832303002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_binding.html:49: // promise_test(async () => { Are you planning to un-comment this test? https://codereview.chromium.org/2832303002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_binding.html:94: var calc1 = new math.CalculatorPtr(); Does this test work? These are not associated ptrs.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2832303002/diff/60001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/60001/mojo/public/js/router.j... mojo/public/js/router.js:140: timer.createOneShot(0, (function() { On 2017/04/24 19:04:33, wangjimmy wrote: > TODO(wangjimmy): Check endpoint client still exists here. Done. https://codereview.chromium.org/2832303002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/mojo/associated_binding.html (right): https://codereview.chromium.org/2832303002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_binding.html:34: var integerSenderBinding = new associatedBindings.AssociatedBinding( On 2017/04/24 19:21:49, yzshen1 wrote: > As a local variable, this could be garbage-collected. Sorry, this test wasn't not suppose to be committed in this patch. I did git add -u so I didn't assume it'd be. But I must have accidental added it before. I wanted to push quickly to see the try results for the new changes. Ignore below as well. https://codereview.chromium.org/2832303002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_binding.html:49: // promise_test(async () => { On 2017/04/24 19:21:49, yzshen1 wrote: > Are you planning to un-comment this test? Acknowledged. https://codereview.chromium.org/2832303002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/mojo/associated_binding.html:94: var calc1 = new math.CalculatorPtr(); On 2017/04/24 19:21:49, yzshen1 wrote: > Does this test work? These are not associated ptrs. Acknowledged.
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2832303002/diff/100001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/100001/mojo/public/js/router.... mojo/public/js/router.js:142: this.cachedMessageData.message.getInterfaceId()); Please check whether this.cachedMessageData is null first. Imagine: - there a cached message for endpoint x; - attach x, which posts a task; - detach x and then re-attach x, which post another task; - the first task deals with the cached message; - the second task is run but there is no cached message. https://codereview.chromium.org/2832303002/diff/100001/mojo/public/js/router.... mojo/public/js/router.js:302: Router.prototype.closeEndpointHandle = function(interfaceId, reason) { If there is a cached message for this endpoint. We should discard the message and resume receiving message now. Besides, please add test case for this scenario.
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2832303002/diff/100001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/100001/mojo/public/js/router.... mojo/public/js/router.js:142: this.cachedMessageData.message.getInterfaceId()); On 2017/04/25 19:56:58, yzshen1 wrote: > Please check whether this.cachedMessageData is null first. > Imagine: > - there a cached message for endpoint x; > - attach x, which posts a task; > - detach x and then re-attach x, which post another task; > - the first task deals with the cached message; > - the second task is run but there is no cached message. Done. https://codereview.chromium.org/2832303002/diff/100001/mojo/public/js/router.... mojo/public/js/router.js:302: Router.prototype.closeEndpointHandle = function(interfaceId, reason) { On 2017/04/25 19:56:58, yzshen1 wrote: > If there is a cached message for this endpoint. We should discard the message > and resume receiving message now. > > Besides, please add test case for this scenario. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a few nits. Thanks! https://codereview.chromium.org/2832303002/diff/120001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/120001/mojo/public/js/router.... mojo/public/js/router.js:150: this.cachedMessageData.message, nit: please move the contents of cachedMessageData to a local variable and resumeIncomingMethodCallProcessing() before calling handleIncomingMessage(). Because that method may re-enter the router, it is a little more robust to do things this way. https://codereview.chromium.org/2832303002/diff/120001/mojo/public/js/router.... mojo/public/js/router.js:324: timer.createOneShot(0, (function() { no need to use timer: resumeIncomingMethodCallProcessing() won't receive message synchronously. https://codereview.chromium.org/2832303002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html (right): https://codereview.chromium.org/2832303002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html:83: function SenderConnectionBindLaterImpl(callbacks = []) { It seems easier to read if we use two parameters "getIntegerSenderCallback" and "getStringSenderCallback".
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2832303002/diff/120001/mojo/public/js/router.js File mojo/public/js/router.js (right): https://codereview.chromium.org/2832303002/diff/120001/mojo/public/js/router.... mojo/public/js/router.js:150: this.cachedMessageData.message, On 2017/04/26 16:40:00, yzshen1 wrote: > nit: please move the contents of cachedMessageData to a local variable and > resumeIncomingMethodCallProcessing() before calling handleIncomingMessage(). > Because that method may re-enter the router, it is a little more robust to do > things this way. Done. https://codereview.chromium.org/2832303002/diff/120001/mojo/public/js/router.... mojo/public/js/router.js:324: timer.createOneShot(0, (function() { On 2017/04/26 16:40:00, yzshen1 wrote: > no need to use timer: resumeIncomingMethodCallProcessing() won't receive message > synchronously. Done. https://codereview.chromium.org/2832303002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html (right): https://codereview.chromium.org/2832303002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/mojo/associated_interface_ptr.html:83: function SenderConnectionBindLaterImpl(callbacks = []) { On 2017/04/26 16:40:00, yzshen1 wrote: > It seems easier to read if we use two parameters "getIntegerSenderCallback" and > "getStringSenderCallback". Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangjimmy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2832303002/#ps140001 (title: "Remove timer from closeEndpointHandle. Clean up associated_interface_ptr.html test.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493236945218540,
"parent_rev": "1e2e347f957ef889aaee527bb757849f76e8a808", "commit_rev":
"d65d570dafc96e2f12abe682c7148a350a94dc52"}
Message was sent while issue was closed.
Description was changed from ========== Fifo order should be preserved for messages on associated interfaces. Cache the current message and pause processing the incoming messages if an endpoint does not have client attached yet. Resume processing the incoming messages when the endpoint client is attached to that endpoint. // Expect arrival of message with value 456, then message with value 123. integerSender1.send(456) // Binding side not bound yet so no endpoint client. integerSender0.send(123) BUG=695635 ========== to ========== Fifo order should be preserved for messages on associated interfaces. Cache the current message and pause processing the incoming messages if an endpoint does not have client attached yet. Resume processing the incoming messages when the endpoint client is attached to that endpoint. // Expect arrival of message with value 456, then message with value 123. integerSender1.send(456) // Binding side not bound yet so no endpoint client. integerSender0.send(123) BUG=695635 Review-Url: https://codereview.chromium.org/2832303002 Cr-Commit-Position: refs/heads/master@{#467427} Committed: https://chromium.googlesource.com/chromium/src/+/d65d570dafc96e2f12abe682c714... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/d65d570dafc96e2f12abe682c714... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
