Cleanup channel dispatchers

remoting/protocol/channel_dispatcher_base.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/protocol/channel_dispatcher_base.h"
 
 #include "base/bind.h"
 #include "net/socket/stream_socket.h"
 #include "remoting/protocol/session.h"
 #include "remoting/protocol/session_config.h"
 #include "remoting/protocol/stream_channel_factory.h"
 
 namespace remoting {
 namespace protocol {
 
 ChannelDispatcherBase::ChannelDispatcherBase(const char* channel_name)
     : channel_name_(channel_name),
-      channel_factory_(nullptr) {
+      channel_factory_(nullptr),
+      event_handler_(nullptr) {
 }
 
 ChannelDispatcherBase::~ChannelDispatcherBase() {
+  writer()->Close();
   if (channel_factory_)
     channel_factory_->CancelChannelCreation(channel_name_);
 }
 
 void ChannelDispatcherBase::Init(Session* session,
                                  const ChannelConfig& config,
-                                 const InitializedCallback& callback) {
+                                 EventHandler* event_handler) {
   DCHECK(session);
   switch (config.transport) {
     case ChannelConfig::TRANSPORT_MUX_STREAM:
       channel_factory_ = session->GetMultiplexedChannelFactory();
       break;
 
     case ChannelConfig::TRANSPORT_STREAM:
       channel_factory_ = session->GetTransportChannelFactory();
       break;
 
     default:
-      NOTREACHED();
-      callback.Run(false);
-      return;
+      LOG(FATAL) << "Unknown transport type: " << config.transport;
   }
 
-  initialized_callback_ = callback;
+  event_handler_ = event_handler;
 
   channel_factory_->CreateChannel(channel_name_, base::Bind(
       &ChannelDispatcherBase::OnChannelReady, base::Unretained(this)));
 }
 
 void ChannelDispatcherBase::OnChannelReady(
     scoped_ptr<net::StreamSocket> socket) {
   if (!socket.get()) {
-    initialized_callback_.Run(false);
+    event_handler_->OnChannelError(this, CHANNEL_CONNECTION_ERROR);
     return;
   }
 
   channel_factory_ = nullptr;
   channel_ = socket.Pass();
+  writer_.Init(channel_.get(), base::Bind(&ChannelDispatcherBase::OnWriteFailed,

[rmsousa, 2015/01/08 02:22:31]

This means that every child of ChannelDispatcherBase will now start both a
reader (that discards everything because the callback is null) and a writer.

Questions:
* Is there such a thing as a read-only/write-only channel, and would this cause
a problem in that case?
* As far as I can tell, each ChannelDispatcherBase creates its own channel -
but, just to confirm, there's no case where we create separate "reader" and
"writer" ChannelDispatchers for hte same channel, right? (i.e. there's no chance
that a "writer" ChannelDispatcherBase would read and discard messages that
another "reader" ChannelDispatcherBase would be interested in)?
* Any other unintended consequences of running a reader in channels wehre we
didn't previously run one? (I assume running hte writer is fine as long as we
don't write anything).

[Sergey Ulanov, 2015/01/08 19:20:32]

Video and Audio channels are currently used in only one direction
Right. ChannelDispatcherBase owns the channel, and there is no way to access it
from somewhere else.
The main difference is that if for some reason a peer sends some data where it's
not expected the receiver will now read and discard the data instead of just
holding it the TCP buffers as it was before. This behavior actually makes
handling of unknown messages more consistent - dispatchers normally drop
messages they do not understand.

+                                          base::Unretained(this)));
+  reader_.StartReading(channel_.get());
 
-  OnInitialized();
+  event_handler_->OnChannelInitialized(this);
+}
 
-  initialized_callback_.Run(true);
+void ChannelDispatcherBase::OnWriteFailed(int error) {
+  event_handler_->OnChannelError(this, CHANNEL_CONNECTION_ERROR);
 }
 
 }  // namespace protocol
 }  // namespace remoting