Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1909143002:
Use ConnectionErrorObserver, not callbacks, for error handling. (Closed)
|
Created:
4 years, 8 months ago by Kevin M Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse ConnectionErrorObserver, not callbacks, for error handling.
The current implementation invokes error observers and a callback
for read and write errors. The responsibility for cleanup and
object deletion was therefore ambiguous, making the callback
and observer clash in certain conditions.
This CL discards the callback entirely in favor of exclusive use of
the error observer for signaling errors.
R=wez@chromium.org
CC=khushalsagar@chromium.org
BUG=605294
Committed: https://crrev.com/793dac0c76b5f43baea388f71bdb5ff0929be915
Cr-Commit-Position: refs/heads/master@{#396294}
Patch Set 1 #
Total comments: 14
Patch Set 2 : wez feedback #
Total comments: 6
Patch Set 3 : wez feedback #Patch Set 4 : minor diff fix #Patch Set 5 : Addressed haibinlu feedback. #
Total comments: 2
Patch Set 6 : wez feedback #Patch Set 7 : Fixed BUILD.gn #Patch Set 8 : rebase #
Messages
Total messages: 30 (11 generated)
https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.cc File blimp/net/blimp_connection.cc (right): https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.... blimp/net/blimp_connection.cc:97: return; This changes the MessageSender to have different semantics to every other MessageProcessor wrt failures, which seems bogus. Isn't the problem here that the caller is deleting the connection from within the context of an observer callback? i.e. it's doing something other than just "observing"? https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.... blimp/net/blimp_connection.cc:110: DCHECK(writer_); Why do we only DCHECK(writer_), but not reader_? https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.... blimp/net/blimp_connection.cc:112: message_pump_->set_error_observer(this); Seems strange that this observer is set on the pump rather than on the reader, since we set an observer on the writer below. https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection_... File blimp/net/blimp_connection_unittest.cc (left): https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection_... blimp/net/blimp_connection_unittest.cc:135: EXPECT_EQ(net::ERR_FAILED, complete_cb_2.WaitForResult()); Can we assert that the second callback has not been called, here? https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_message_out... File blimp/net/blimp_message_output_buffer.cc (right): https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_message_out... blimp/net/blimp_message_output_buffer.cc:147: DCHECK_EQ(result, net::OK); It seems that you're changing the MessageProcessor contract such that processing a message can never fail, so why do we still have |result| in the callback signature? nit: DCHECK_EQ(expected, actual) is equivalent but preferred, since it matches the gmock EXPECT_EQ() required ordering. nit: Functions like this are easier to read if you use blank lines to e.g. separate the pre-condition checks (this DCHECK) from the body, and generally to break up the body into logically blocks. https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_message_out... blimp/net/blimp_message_output_buffer.cc:148: VLOG(2) << "Write complete."; nit: None of our other log messages, above, seem to have terminating periods.
https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.cc File blimp/net/blimp_connection.cc (right): https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.... blimp/net/blimp_connection.cc:97: return; On 2016/04/21 22:44:29, Wez wrote: > This changes the MessageSender to have different semantics to every other > MessageProcessor wrt failures, which seems bogus. > > Isn't the problem here that the caller is deleting the connection from within > the context of an observer callback? i.e. it's doing something other than just > "observing"? Isn't it normal for an observer of an object to have side effects, including destruction of said object? https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.... blimp/net/blimp_connection.cc:110: DCHECK(writer_); On 2016/04/21 22:44:29, Wez wrote: > Why do we only DCHECK(writer_), but not reader_? Done. https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.... blimp/net/blimp_connection.cc:112: message_pump_->set_error_observer(this); On 2016/04/21 22:44:29, Wez wrote: > Seems strange that this observer is set on the pump rather than on the reader, > since we set an observer on the writer below. The use of observers is consistent here. The sender is distinct from the writer; BlimpMessageSender converts BlimpMessage to bytes before pushing them to a writer. Likewise, the pump takes bytes from a reader and converts them to BlimpMessages. https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection_... File blimp/net/blimp_connection_unittest.cc (left): https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection_... blimp/net/blimp_connection_unittest.cc:135: EXPECT_EQ(net::ERR_FAILED, complete_cb_2.WaitForResult()); On 2016/04/21 22:44:29, Wez wrote: > Can we assert that the second callback has not been called, here? Done. https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_message_out... File blimp/net/blimp_message_output_buffer.cc (right): https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_message_out... blimp/net/blimp_message_output_buffer.cc:147: DCHECK_EQ(result, net::OK); On 2016/04/21 22:44:29, Wez wrote: > It seems that you're changing the MessageProcessor contract such that processing > a message can never fail, so why do we still have |result| in the callback > signature? > > nit: DCHECK_EQ(expected, actual) is equivalent but preferred, since it matches > the gmock EXPECT_EQ() required ordering. > > nit: Functions like this are easier to read if you use blank lines to e.g. > separate the pre-condition checks (this DCHECK) from the body, and generally to > break up the body into logically blocks. Done. https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_message_out... blimp/net/blimp_message_output_buffer.cc:148: VLOG(2) << "Write complete."; On 2016/04/21 22:44:29, Wez wrote: > nit: None of our other log messages, above, seem to have terminating periods. Done.
kmarshall@chromium.org changed reviewers: + haibinlu@chromium.org - wez@chromium.org
wez=>haibin
https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_connect... File blimp/net/blimp_connection.cc (right): https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_connect... blimp/net/blimp_connection.cc:100: base::ResetAndReturn(&pending_process_msg_callback_).Run(result); nit. use net::OK instead to be specific. Also, add comment about when callers can expect callback to always have net::OK. https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_message... File blimp/net/blimp_message_output_buffer.cc (right): https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.cc:148: DCHECK_EQ(net::OK, result)
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.cc File blimp/net/blimp_connection.cc (right): https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.... blimp/net/blimp_connection.cc:97: return; On 2016/04/22 22:10:41, Kevin M wrote: > On 2016/04/21 22:44:29, Wez wrote: > > This changes the MessageSender to have different semantics to every other > > MessageProcessor wrt failures, which seems bogus. > > > > Isn't the problem here that the caller is deleting the connection from within > > the context of an observer callback? i.e. it's doing something other than > just > > "observing"? > > Isn't it normal for an observer of an object to have side effects, including > destruction of said object? In most cases I'd expect that an observer might tear down other auxiliary objects, but not the object it's observing; it's common to restrict what observers are permitted to do. The alternate, if you want to continue to allow the observer to delete |this|, would be to move the pending-message callback into a stack variable, call the observer, then call the pending-message callback from the stack - caller can Bind() the pending-message callback to a WeakPtr to allow it to become a no-op in case of the caller having deleted stuff in the observer. https://codereview.chromium.org/1909143002/diff/1/blimp/net/blimp_connection.... blimp/net/blimp_connection.cc:112: message_pump_->set_error_observer(this); On 2016/04/22 22:10:41, Kevin M wrote: > On 2016/04/21 22:44:29, Wez wrote: > > Seems strange that this observer is set on the pump rather than on the reader, > > since we set an observer on the writer below. > > The use of observers is consistent here. The sender is distinct from the writer; > BlimpMessageSender converts BlimpMessage to bytes before pushing them to a > writer. > > Likewise, the pump takes bytes from a reader and converts them to BlimpMessages. Acknowledged. https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_message... File blimp/net/blimp_message_output_buffer.cc (right): https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.cc:148: On 2016/05/03 21:23:05, haibinlu wrote: > DCHECK_EQ(net::OK, result) See my comment on the earlier review ;)
Pingy; can we get this finished-up and landed?
https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_connect... File blimp/net/blimp_connection.cc (right): https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_connect... blimp/net/blimp_connection.cc:100: base::ResetAndReturn(&pending_process_msg_callback_).Run(result); On 2016/05/03 21:23:05, haibinlu wrote: > nit. use net::OK instead to be specific. > > Also, add comment about when callers can expect callback to always have net::OK. Done. https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_message... File blimp/net/blimp_message_output_buffer.cc (right): https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.cc:148: On 2016/05/07 22:13:47, Wez wrote: > On 2016/05/03 21:23:05, haibinlu wrote: > > DCHECK_EQ(net::OK, result) > > See my comment on the earlier review ;) Sorry, was that in reference to the callback signature? I see that I didn't address that point. I take "result" because CompletionCallbacks are part of the BlimpMessageProcessor convention. I also restored the check for result == net::OK https://codereview.chromium.org/1909143002/diff/20001/blimp/net/blimp_message... blimp/net/blimp_message_output_buffer.cc:148: On 2016/05/03 21:23:05, haibinlu wrote: > DCHECK_EQ(net::OK, result) I added a DCHECK for positive values.
lgtm
LGTM Presumably things go horrible wrong if multiple Observers try to DropConnection()? https://codereview.chromium.org/1909143002/diff/80001/blimp/net/blimp_connect... File blimp/net/blimp_connection_unittest.cc (right): https://codereview.chromium.org/1909143002/diff/80001/blimp/net/blimp_connect... blimp/net/blimp_connection_unittest.cc:126: this, &BlimpConnectionTest::DropConnection)); You don't want to add a separate, dedicated test?
> Presumably things go horrible wrong if multiple Observers try to DropConnection()? Correct - only an owner that's observing the connection may drop it. https://codereview.chromium.org/1909143002/diff/80001/blimp/net/blimp_connect... File blimp/net/blimp_connection_unittest.cc (right): https://codereview.chromium.org/1909143002/diff/80001/blimp/net/blimp_connect... blimp/net/blimp_connection_unittest.cc:126: this, &BlimpConnectionTest::DropConnection)); On 2016/05/25 03:28:35, Wez wrote: > You don't want to add a separate, dedicated test? Done.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haibinlu@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1909143002/#ps100001 (title: "wez feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1909143002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1909143002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haibinlu@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1909143002/#ps120001 (title: "Fixed BUILD.gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1909143002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1909143002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haibinlu@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1909143002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1909143002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1909143002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Use ConnectionErrorObserver, not callbacks, for error handling. The current implementation invokes error observers and a callback for read and write errors. The responsibility for cleanup and object deletion was therefore ambiguous, making the callback and observer clash in certain conditions. This CL discards the callback entirely in favor of exclusive use of the error observer for signaling errors. R=wez@chromium.org CC=khushalsagar@chromium.org BUG=605294 ========== to ========== Use ConnectionErrorObserver, not callbacks, for error handling. The current implementation invokes error observers and a callback for read and write errors. The responsibility for cleanup and object deletion was therefore ambiguous, making the callback and observer clash in certain conditions. This CL discards the callback entirely in favor of exclusive use of the error observer for signaling errors. R=wez@chromium.org CC=khushalsagar@chromium.org BUG=605294 Committed: https://crrev.com/793dac0c76b5f43baea388f71bdb5ff0929be915 Cr-Commit-Position: refs/heads/master@{#396294} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/793dac0c76b5f43baea388f71bdb5ff0929be915 Cr-Commit-Position: refs/heads/master@{#396294} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
