Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770153003:
mojo: Support sync calls through ThreadSafeInterfacePtr (Closed)
Descriptionmojo: Support sync calls through ThreadSafeInterfacePtr
Previously ThreadSafeInterfacePtr only supported async calls. Now it
also supports sync calls.
The one caveat to be aware of is that sync calls will block both the
calling thread and the thread that the underlying InterfacePtr is bound
to. That means, e.g., that the InterfacePtr can't be bound to the IO
thread. Letting the call be async on the InterfacePtr thread is left as
a TODO.
BUG=668565
Review-Url: https://codereview.chromium.org/2770153003
Cr-Commit-Position: refs/heads/master@{#461016}
Committed: https://chromium.googlesource.com/chromium/src/+/0c3ac0678cbc511f793dfd7386729b1b2ae2e7e1
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix more tests #
Total comments: 21
Patch Set 3 : make it threadsafe #
Total comments: 10
Patch Set 4 : Addressed comments #Patch Set 5 : SyncMethodCommonTests now run with both IP and TSIP #
Total comments: 8
Patch Set 6 : afds #
Messages
Total messages: 30 (12 generated)
watk@chromium.org changed reviewers: + yzshen@chromium.org
Hey Yuzhu, I wanted to share my current WIP to get some feedback from you, i.e., how broken is this? :) I have no idea how it interacts with associated interfaces, and what not. The main changes are in ThreadSafeForwarder::AcceptWithResponder. You can ignore the test changes. https://codereview.chromium.org/2770153003/diff/1/mojo/public/cpp/bindings/te... File mojo/public/cpp/bindings/tests/sync_method_unittest.cc (right): https://codereview.chromium.org/2770153003/diff/1/mojo/public/cpp/bindings/te... mojo/public/cpp/bindings/tests/sync_method_unittest.cc:402: TYPED_TEST(SyncMethodCommonTest, TSIPInterfacePtrDestroyedDuringSyncCall) { I just added these for local testing. I'm thinking it's probably possible to make ThreadSafeInterfacePtr another dimension of the tests params, so we'd run all these with both IP and TSIP. WDYT?
Description was changed from ========== WIP: Add ability to make sync calls through ThreadSafeInterfacePtr BUG= ========== to ========== WIP: Add ability to make sync calls through ThreadSafeInterfacePtr BUG= ==========
yzshen@chromium.org changed reviewers: + rockot@chromium.org
> I'm thinking it's probably possible to make > ThreadSafeInterfacePtr another > dimension of the tests params, so we'd run all these with > both IP and TSIP. > WDYT? Sounds good if it is not too hard. :) https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:22: namespace { It is pretty uncommon and not recommended to use anonymous namespace in header files. How about making it a static member of ThreadSafeForwarder? https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:99: forward_with_responder_.Run(std::move(*message), I think we would like to preserve order between calls, and therefore we don't directly run |forward_with_responder_| even if on the same thread. EXAMPLE: Imagine thread_a uses ThreadSafeForwarder to make a call X, and then signals (using some way other than posting a task) thread_b to use the same forwarder to make a call Y. The forwarder's |task_runner_| is bound to thread_b. If we pass the Y call directly to the interface pointer, it is possible that Y happens prior to X. https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:112: forward_with_responder_.Run(std::move(*message), std::move(responder)); +CC Ken: At the beginning, I thought this direct call would have the same issue as the one in the async case. But after some more thoughts, it seems fine: First, we don't care about ordering between sync calls and async calls, because they could be out-of-order due to re-entrancy anyway. If the order between two sync calls made from a ThreadSafeInterfacePtr is well-defined, in all the cases that I can think up, the underlying interface ptr will run those two calls in order. WDYT? Have I missed anything obvious? https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:126: // on the same sync_message_event_? Correct because only reentrant sync I am not quite sure I understand the question. Would you please explain? https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:130: watcher.AllowWokenUpBySyncWatchOnSameThread(); I don't think you need this line. This is for the implementation of interfaces that are listening for sync requests on the same thread. They may need to be woken up while this interface pointer is waiting for the sync call response. https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:133: auto weak_self = weak_factory_.GetWeakPtr(); WeakPtrFactory will force this object to be only used on a specific thread. If all we need is to track whether |this| is destroyed. You could use something like https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/sync_handle_wat... https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:138: ignore_result(responder->Accept(&response)); The response may not correspond to the |responder|. Example: 1) ptr_A make sync call X; 2) while waiting for X's response, an incoming sync request for another interface B renters this thread 3) while interface B handles this request, it uses ptr_A to make sync call Y; 4) the X's response arrive, in this case it will be incorrectly considered as response for Y. That is why InterfaceEndpointClient needs to have a SyncResponseMap. You could take a look at that. https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:158: // XXX: What guarantees do we have about the lifetime of these? The sync_response_event_ may be destroyed in the middle, right? Example: 1) forwarder_A is used to make sync call X; 2) while waiting for X's response, an incoming sync request for another interface B renters this thread; 3) while interface B handles this request, it destroys forwarder_A; 4) if the underlying interface pointer is kept alive, it holds a SyncReplySignaler with invalid pointer.
I appreciate the comments, this is pretty subtle. Replying without a new patch set in case you had further comments because going home for the day :) https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:22: namespace { On 2017/03/27 22:18:25, yzshen1 wrote: > It is pretty uncommon and not recommended to use anonymous namespace in header > files. > > How about making it a static member of ThreadSafeForwarder? > Oops, forgot this was a header. https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:99: forward_with_responder_.Run(std::move(*message), On 2017/03/27 22:18:24, yzshen1 wrote: > I think we would like to preserve order between calls, and therefore we don't > directly run |forward_with_responder_| even if on the same thread. > > EXAMPLE: > Imagine thread_a uses ThreadSafeForwarder to make a call X, and then signals > (using some way other than posting a task) thread_b to use the same forwarder to > make a call Y. The forwarder's |task_runner_| is bound to thread_b. > > If we pass the Y call directly to the interface pointer, it is possible that Y > happens prior to X. True. I had assumed there was a contract that TSIP is to be used on one thread only, but that was a false assumption. Hmm, this is tricky then. I added this to preserve the invariant that queued async resonses are dispatched before the error notification. If we always post, that is broken. So either that's not worth trying to preserve for TSIP, or I should find another way. I'll default to the latter if you have no thoughts. https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:112: forward_with_responder_.Run(std::move(*message), std::move(responder)); On 2017/03/27 22:18:24, yzshen1 wrote: > +CC Ken: > > At the beginning, I thought this direct call would have the same issue as the > one in the async case. But after some more thoughts, it seems fine: > First, we don't care about ordering between sync calls and async calls, because > they could be out-of-order due to re-entrancy anyway. > > If the order between two sync calls made from a ThreadSafeInterfacePtr is > well-defined, in all the cases that I can think up, the underlying interface ptr > will run those two calls in order. > > WDYT? Have I missed anything obvious? > I did this because I wasn't sure what the alternative would be, i.e., we can't post the task because we have to SyncWatch for the response on the same thread. Oh, we could run a nested message loop? https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:126: // on the same sync_message_event_? Correct because only reentrant sync On 2017/03/27 22:18:25, yzshen1 wrote: > I am not quite sure I understand the question. Would you please explain? I think I wrote this before I had the sync_message_event_.Reset() below and I wasn't sure if it was correct that a nested sync call completing would also wake up the outer sync call. I don't think this is a concern any more because the event is reset. So the outer sync call will need to be signaled separately from the inner one. https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:130: watcher.AllowWokenUpBySyncWatchOnSameThread(); On 2017/03/27 22:18:24, yzshen1 wrote: > I don't think you need this line. > > This is for the implementation of interfaces that are listening for sync > requests on the same thread. They may need to be woken up while this interface > pointer is waiting for the sync call response. Ah, gotcha. https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:133: auto weak_self = weak_factory_.GetWeakPtr(); On 2017/03/27 22:18:24, yzshen1 wrote: > WeakPtrFactory will force this object to be only used on a specific thread. > > If all we need is to track whether |this| is destroyed. You could use something > like > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/sync_handle_wat... SG, thanks https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:138: ignore_result(responder->Accept(&response)); On 2017/03/27 22:18:24, yzshen1 wrote: > The response may not correspond to the |responder|. > > Example: > > 1) ptr_A make sync call X; > 2) while waiting for X's response, an incoming sync request for another > interface B renters this thread > 3) while interface B handles this request, it uses ptr_A to make sync call Y; > 4) the X's response arrive, in this case it will be incorrectly considered as > response for Y. > > That is why InterfaceEndpointClient needs to have a SyncResponseMap. You could > take a look at that. Ah, yes, thanks. I hadn't even thought about non-nested sync calls :O https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:158: // XXX: What guarantees do we have about the lifetime of these? On 2017/03/27 22:18:25, yzshen1 wrote: > The sync_response_event_ may be destroyed in the middle, right? > > Example: > > 1) forwarder_A is used to make sync call X; > 2) while waiting for X's response, an incoming sync request for another > interface B renters this thread; > 3) while interface B handles this request, it destroys forwarder_A; > 4) if the underlying interface pointer is kept alive, it holds a > SyncReplySignaler with invalid pointer. Yes, true. I'll make this refcounted then.
https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:99: forward_with_responder_.Run(std::move(*message), On 2017/03/27 23:45:29, watk wrote: > On 2017/03/27 22:18:24, yzshen1 wrote: > > I think we would like to preserve order between calls, and therefore we don't > > directly run |forward_with_responder_| even if on the same thread. > > > > EXAMPLE: > > Imagine thread_a uses ThreadSafeForwarder to make a call X, and then signals > > (using some way other than posting a task) thread_b to use the same forwarder > to > > make a call Y. The forwarder's |task_runner_| is bound to thread_b. > > > > If we pass the Y call directly to the interface pointer, it is possible that Y > > happens prior to X. > > True. I had assumed there was a contract that TSIP is to be used on one thread > only, but that was a false assumption. Hmm, this is tricky then. I added this to > preserve the invariant that queued async resonses are dispatched before the > error notification. If we always post, that is broken. So either that's not > worth trying to preserve for TSIP, or I should find another way. I'll default to > the latter if you have no thoughts. What error notification? :) https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:112: forward_with_responder_.Run(std::move(*message), std::move(responder)); On 2017/03/27 23:45:29, watk wrote: > On 2017/03/27 22:18:24, yzshen1 wrote: > > +CC Ken: > > > > At the beginning, I thought this direct call would have the same issue as the > > one in the async case. But after some more thoughts, it seems fine: > > First, we don't care about ordering between sync calls and async calls, > because > > they could be out-of-order due to re-entrancy anyway. > > > > If the order between two sync calls made from a ThreadSafeInterfacePtr is > > well-defined, in all the cases that I can think up, the underlying interface > ptr > > will run those two calls in order. > > > > WDYT? Have I missed anything obvious? > > > > I did this because I wasn't sure what the alternative would be, i.e., we can't > post the task because we have to SyncWatch for the response on the same thread. > Oh, we could run a nested message loop? What I was trying to say is "I thought this doesn't work but after some more thoughts I think you are doing the right thing". :)
https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:99: forward_with_responder_.Run(std::move(*message), On 2017/03/28 00:06:48, yzshen1 wrote: > On 2017/03/27 23:45:29, watk wrote: > > On 2017/03/27 22:18:24, yzshen1 wrote: > > > I think we would like to preserve order between calls, and therefore we > don't > > > directly run |forward_with_responder_| even if on the same thread. > > > > > > EXAMPLE: > > > Imagine thread_a uses ThreadSafeForwarder to make a call X, and then signals > > > (using some way other than posting a task) thread_b to use the same > forwarder > > to > > > make a call Y. The forwarder's |task_runner_| is bound to thread_b. > > > > > > If we pass the Y call directly to the interface pointer, it is possible that > Y > > > happens prior to X. > > > > True. I had assumed there was a contract that TSIP is to be used on one thread > > only, but that was a false assumption. Hmm, this is tricky then. I added this > to > > preserve the invariant that queued async resonses are dispatched before the > > error notification. If we always post, that is broken. So either that's not > > worth trying to preserve for TSIP, or I should find another way. I'll default > to > > the latter if you have no thoughts. > > What error notification? :) Take a look at sync_method_unittest.cc line 853. That's the test that motivated not always posting async messages. https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:112: forward_with_responder_.Run(std::move(*message), std::move(responder)); On 2017/03/28 00:06:48, yzshen1 wrote: > On 2017/03/27 23:45:29, watk wrote: > > On 2017/03/27 22:18:24, yzshen1 wrote: > > > +CC Ken: > > > > > > At the beginning, I thought this direct call would have the same issue as > the > > > one in the async case. But after some more thoughts, it seems fine: > > > First, we don't care about ordering between sync calls and async calls, > > because > > > they could be out-of-order due to re-entrancy anyway. > > > > > > If the order between two sync calls made from a ThreadSafeInterfacePtr is > > > well-defined, in all the cases that I can think up, the underlying interface > > ptr > > > will run those two calls in order. > > > > > > WDYT? Have I missed anything obvious? > > > > > > > I did this because I wasn't sure what the alternative would be, i.e., we can't > > post the task because we have to SyncWatch for the response on the same > thread. > > Oh, we could run a nested message loop? > > What I was trying to say is "I thought this doesn't work but after some more > thoughts I think you are doing the right thing". :) Oh, ok :)
https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/tests/sync_method_unittest.cc (right): https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/tests/sync_method_unittest.cc:854: TSIPQueuedMessagesProcessedBeforeErrorNotification) { Because connection error handler is not exposed by TSIP. We didn't make any guarantee between messages received by TSIP and the error notification received by the underlying IP. But I agree that it is unfortunate. But otherwise this thing will become more and more similar to MultiplexRouter, which is pretty complex. :/ One idea that Ken and I chatted about is maybe we could support "cloning" an interface pointer as an associated interface pointer, so it can be used on another thread. In that case, MultiplexRouter will do all the work -- sync calls, strict order guarantee, etc. (But I am fine landing the current CL and consider that later).
On 2017/03/28 15:56:03, yzshen1 wrote: > https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... > File mojo/public/cpp/bindings/tests/sync_method_unittest.cc (right): > > https://codereview.chromium.org/2770153003/diff/20001/mojo/public/cpp/binding... > mojo/public/cpp/bindings/tests/sync_method_unittest.cc:854: > TSIPQueuedMessagesProcessedBeforeErrorNotification) { > Because connection error handler is not exposed by TSIP. We didn't make any > guarantee between messages received by TSIP and the error notification received > by the underlying IP. > > But I agree that it is unfortunate. But otherwise this thing will become more > and more similar to MultiplexRouter, which is pretty complex. :/ > > One idea that Ken and I chatted about is maybe we could support "cloning" an > interface pointer as an associated interface pointer, so it can be used on > another thread. In that case, MultiplexRouter will do all the work -- sync > calls, strict order guarantee, etc. (But I am fine landing the current CL and > consider that later). Ok, makes sense, that definitely makes this easier :) Using an associated pointer sounds like a great idea. I'll land this approach for now as you suggest, because I won't have time to try the associated pointer one.
Ok, uploaded a new patchset for feedback (I only have two days left before I'm on vacation :() In this one I used one WaitableEvent and SyncEventWatcher per sync call. (The old way wasn't threadsafe). This lets us avoid having to use message_ids and a map of sync calls I believe. The handling of a sync call deleting |this| was a bit weird. I'm not sure if it's correct. If this looks alright, the remaining work is to add a bunch of tests.
https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:61: pending_sync_responses_.front()->event.Signal(); Shouldn't we signal all the events? https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:118: // TODO(yzshen, watk): We block both this thread and the InterfacePtr IO thread must not be blocked. This prevents users from using sync calls with ThreadSafeInterfacePtr whose InterfacePtr lives on IO thread. That being said, I am fine leaving it as a TODO. https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:145: base::AutoLock l(lock_); The object may be destroyed, so it is not safe to access the member variables without checking, right? https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:153: struct SyncResponseInfo { nit: because you have full control over this type, you could directly make it to support ref-counted, instead of using RefCountedData
Thanks! I'm going to focus on tests. I tried making "use_tsip" a parameter of the test fixture template, but I didn't like the indirection through templated Traits functions and the extra "typename"s needed. It obfuscated the tests too much. I'll start by getting tests written and see about reducing duplication later. https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:61: pending_sync_responses_.front()->event.Signal(); On 2017/03/29 23:16:47, yzshen1 wrote: > Shouldn't we signal all the events? Done. https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:118: // TODO(yzshen, watk): We block both this thread and the InterfacePtr On 2017/03/29 23:16:47, yzshen1 wrote: > IO thread must not be blocked. This prevents users from using sync calls with > ThreadSafeInterfacePtr whose InterfacePtr lives on IO thread. > > That being said, I am fine leaving it as a TODO. Yeah this is a shame :( I just ran out of time to implement that. I suspect there's some trickiness there. https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:145: base::AutoLock l(lock_); On 2017/03/29 23:16:47, yzshen1 wrote: > The object may be destroyed, so it is not safe to access the member variables > without checking, right? Oops! Made it refcounted. https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:153: struct SyncResponseInfo { On 2017/03/29 23:16:47, yzshen1 wrote: > nit: because you have full control over this type, you could directly make it to > support ref-counted, instead of using RefCountedData Done.
Description was changed from ========== WIP: Add ability to make sync calls through ThreadSafeInterfacePtr BUG= ========== to ========== mojo: Support sync calls through ThreadSafeInterfacePtr Previously ThreadSafeInterfacePtr only supported async calls. Now it also supports sync calls. The one caveat to be aware of is that sync calls will block both the calling thread and the thread that the underlying InterfacePtr is bound to. That means, e.g., that the InterfacePtr can't be bound to the IO thread. Letting the call be async on the InterfacePtr thread is left as a TODO. BUG=668565 ==========
Patchset #5 (id:80001) has been deleted
Ok, I think these tests look reasonable. SyncMethodCommonTests now run with IP and TSIP. I'll add some more tests tomorrow. e.g., concurrent sync calls.
Only a few nits. Thanks! https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:118: // TODO(yzshen, watk): We block both this thread and the InterfacePtr On 2017/03/30 02:26:05, watk wrote: > On 2017/03/29 23:16:47, yzshen1 wrote: > > IO thread must not be blocked. This prevents users from using sync calls with > > ThreadSafeInterfacePtr whose InterfacePtr lives on IO thread. > > > > That being said, I am fine leaving it as a TODO. > > Yeah this is a shame :( I just ran out of time to implement that. I suspect > there's some trickiness there. Please document this restriction WRT IO thread in the comment. https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/tests/sync_method_unittest.cc (right): https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/tests/sync_method_unittest.cc:244: static const bool is_thread_safe_ptr_test = use_thread_safe_ptr; Because this is a compile time constant, I think it should be kIsThreadSafePtrTest? https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/tests/sync_method_unittest.cc:650: // ThreadSafeInterfacePtr doesn't make guarantees about error notifications. nit: it would be nice to clarify that "doesn't make guarantees that messages are delivered before the error notification". https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/tests/sync_method_unittest.cc:755: if (!TypeParam::is_thread_safe_ptr_test) { I guess this test case works for TSIP because it doesn't rely on the order of messages and error notification. Right? https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:144: if (event_signaled && response->received) Do we need to check |event_signaled|?
https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/40001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:118: // TODO(yzshen, watk): We block both this thread and the InterfacePtr On 2017/03/30 20:53:05, yzshen1 wrote: > On 2017/03/30 02:26:05, watk wrote: > > On 2017/03/29 23:16:47, yzshen1 wrote: > > > IO thread must not be blocked. This prevents users from using sync calls > with > > > ThreadSafeInterfacePtr whose InterfacePtr lives on IO thread. > > > > > > That being said, I am fine leaving it as a TODO. > > > > Yeah this is a shame :( I just ran out of time to implement that. I suspect > > there's some trickiness there. > > Please document this restriction WRT IO thread in the comment. I added a file-level comment. Is that OK? https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/tests/sync_method_unittest.cc (right): https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/tests/sync_method_unittest.cc:244: static const bool is_thread_safe_ptr_test = use_thread_safe_ptr; On 2017/03/30 20:53:06, yzshen1 wrote: > Because this is a compile time constant, I think it should be > kIsThreadSafePtrTest? Done. https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/tests/sync_method_unittest.cc:650: // ThreadSafeInterfacePtr doesn't make guarantees about error notifications. On 2017/03/30 20:53:06, yzshen1 wrote: > nit: it would be nice to clarify that "doesn't make guarantees that messages are > delivered before the error notification". Done. https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/tests/sync_method_unittest.cc:755: if (!TypeParam::is_thread_safe_ptr_test) { On 2017/03/30 20:53:06, yzshen1 wrote: > I guess this test case works for TSIP because it doesn't rely on the order of > messages and error notification. Right? Well for TSIP the value of this test is that invalid messages result in sync calls returning false. Since TSIP doesn't let you set a connection error handler I didn't bother running that bit for TSIP. https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2770153003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:144: if (event_signaled && response->received) On 2017/03/30 20:53:06, yzshen1 wrote: > Do we need to check |event_signaled|? Can't think of a reason. Removed.
The CQ bit was checked by watk@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Just in time before I go on vacation for three weeks :)
The CQ bit was checked by watk@chromium.org
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": 120001, "attempt_start_ts": 1490930945698130,
"parent_rev": "1459bd0dc4107bcde17077387bd5705097d51c44", "commit_rev":
"0c3ac0678cbc511f793dfd7386729b1b2ae2e7e1"}
Message was sent while issue was closed.
Description was changed from ========== mojo: Support sync calls through ThreadSafeInterfacePtr Previously ThreadSafeInterfacePtr only supported async calls. Now it also supports sync calls. The one caveat to be aware of is that sync calls will block both the calling thread and the thread that the underlying InterfacePtr is bound to. That means, e.g., that the InterfacePtr can't be bound to the IO thread. Letting the call be async on the InterfacePtr thread is left as a TODO. BUG=668565 ========== to ========== mojo: Support sync calls through ThreadSafeInterfacePtr Previously ThreadSafeInterfacePtr only supported async calls. Now it also supports sync calls. The one caveat to be aware of is that sync calls will block both the calling thread and the thread that the underlying InterfacePtr is bound to. That means, e.g., that the InterfacePtr can't be bound to the IO thread. Letting the call be async on the InterfacePtr thread is left as a TODO. BUG=668565 Review-Url: https://codereview.chromium.org/2770153003 Cr-Commit-Position: refs/heads/master@{#461016} Committed: https://chromium.googlesource.com/chromium/src/+/0c3ac0678cbc511f793dfd738672... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0c3ac0678cbc511f793dfd738672... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
