Enqueue mDns requests if send on socket in progress.

BUG=415753

Review URL: https://codereview.chromium.org/581813004

Cr-Commit-Position: refs/heads/master@{#296094}

net/dns/mdns_client_impl.cc

@@ -4,6 +4,8 @@
 
 #include "net/dns/mdns_client_impl.h"
 
+#include <queue>
+
 #include "base/bind.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/stl_util.h"
@@ -54,7 +56,8 @@ MDnsConnection::SocketHandler::SocketHandler(
     MDnsConnection* connection)
     : socket_(socket.Pass()),
       connection_(connection),
-      response_(dns_protocol::kMaxMulticastSize) {
+      response_(dns_protocol::kMaxMulticastSize),
+      send_in_progress_(false) {
 }
 
 MDnsConnection::SocketHandler::~SocketHandler() {
@@ -95,21 +98,41 @@ void MDnsConnection::SocketHandler::OnDatagramReceived(int rv) {
     rv = DoLoop(rv);
 
   if (rv != OK)
-    connection_->OnError(this, rv);
+    connection_->PostOnError(this, rv);
 }
 
-int MDnsConnection::SocketHandler::Send(IOBuffer* buffer, unsigned size) {
-  return socket_->SendTo(buffer, size, multicast_addr_,
-                         base::Bind(&MDnsConnection::SocketHandler::SendDone,
-                                    base::Unretained(this) ));
+void MDnsConnection::SocketHandler::Send(const scoped_refptr<IOBuffer>& buffer,
+                                         unsigned size) {
+  if (send_in_progress_) {
+    send_queue_.push(std::make_pair(buffer, size));
+    return;
+  }
+  int rv = socket_->SendTo(buffer.get(),
+                           size,
+                           multicast_addr_,
+                           base::Bind(&MDnsConnection::SocketHandler::SendDone,
+                                      base::Unretained(this)));
+  if (rv == ERR_IO_PENDING) {
+    send_in_progress_ = true;
+  } else if (rv < OK) {
+    connection_->PostOnError(this, rv);
+  }
 }
 
 void MDnsConnection::SocketHandler::SendDone(int rv) {
-  // TODO(noamsml): Retry logic.
+  DCHECK(send_in_progress_);
+  send_in_progress_ = false;
+  if (rv != OK)
+    connection_->PostOnError(this, rv);
+  while (!send_in_progress_ && !send_queue_.empty()) {
+    std::pair<scoped_refptr<IOBuffer>, unsigned> buffer = send_queue_.front();
+    send_queue_.pop();
+    Send(buffer.first, buffer.second);
+  }
 }
 
-MDnsConnection::MDnsConnection(MDnsConnection::Delegate* delegate) :
-      delegate_(delegate) {
+MDnsConnection::MDnsConnection(MDnsConnection::Delegate* delegate)
+    : delegate_(delegate), weak_ptr_factory_(this) {
 }
 
 MDnsConnection::~MDnsConnection() {
@@ -141,24 +164,26 @@ bool MDnsConnection::Init(MDnsSocketFactory* socket_factory) {
   return !socket_handlers_.empty();
 }
 
-bool MDnsConnection::Send(IOBuffer* buffer, unsigned size) {
-  bool success = false;
-  for (size_t i = 0; i < socket_handlers_.size(); ++i) {
-    int rv = socket_handlers_[i]->Send(buffer, size);
-    if (rv >= OK || rv == ERR_IO_PENDING) {
-      success = true;
-    } else {
-      VLOG(1) << "Send failed, socket=" << i << ", error=" << rv;
-    }
-  }
-  return success;
+void MDnsConnection::Send(const scoped_refptr<IOBuffer>& buffer,
+                          unsigned size) {
+  for (size_t i = 0; i < socket_handlers_.size(); ++i)
+    socket_handlers_[i]->Send(buffer, size);
+}
+
+void MDnsConnection::PostOnError(SocketHandler* loop, int rv) {
+  VLOG(1) << "Socket error. id="
+          << std::find(socket_handlers_.begin(), socket_handlers_.end(), loop) -
+                 socket_handlers_.begin() << ", error=" << rv;
+  // Post to allow deletion of this object by delegate.
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&MDnsConnection::OnError, weak_ptr_factory_.GetWeakPtr(), rv));
 }
 
-void MDnsConnection::OnError(SocketHandler* loop,
-                             int error) {
+void MDnsConnection::OnError(int rv) {
   // TODO(noamsml): Specific handling of intermittent errors that can be handled
   // in the connection.
-  delegate_->OnConnectionError(error);
+  delegate_->OnConnectionError(rv);
 }
 
 void MDnsConnection::OnDatagramReceived(
@@ -190,7 +215,8 @@ bool MDnsClientImpl::Core::SendQuery(uint16 rrtype, std::string name) {
   DnsQuery query(0, name_dns, rrtype);
   query.set_flags(0);  // Remove the RD flag from the query. It is unneeded.
 
-  return connection_->Send(query.io_buffer(), query.io_buffer()->size());
+  connection_->Send(query.io_buffer(), query.io_buffer()->size());
+  return true;
 }
 
 void MDnsClientImpl::Core::HandlePacket(DnsResponse* response,

net/dns/mdns_client_impl.h

@@ -52,7 +52,7 @@ class NET_EXPORT_PRIVATE MDnsConnection {
 
   // Both methods return true if at least one of the socket handlers succeeded.
   bool Init(MDnsSocketFactory* socket_factory);
-  bool Send(IOBuffer* buffer, unsigned size);
+  void Send(const scoped_refptr<IOBuffer>& buffer, unsigned size);
 
  private:
   class SocketHandler {
@@ -62,7 +62,7 @@ class NET_EXPORT_PRIVATE MDnsConnection {
     ~SocketHandler();
 
     int Start();
-    int Send(IOBuffer* buffer, unsigned size);
+    void Send(const scoped_refptr<IOBuffer>& buffer, unsigned size);
 
    private:
     int DoLoop(int rv);
@@ -76,6 +76,8 @@ class NET_EXPORT_PRIVATE MDnsConnection {
     IPEndPoint recv_addr_;
     DnsResponse response_;
     IPEndPoint multicast_addr_;
+    bool send_in_progress_;
+    std::queue<std::pair<scoped_refptr<IOBuffer>, unsigned> > send_queue_;
 
     DISALLOW_COPY_AND_ASSIGN(SocketHandler);
   };
@@ -85,13 +87,16 @@ class NET_EXPORT_PRIVATE MDnsConnection {
                           const IPEndPoint& recv_addr,
                           int bytes_read);
 
-  void OnError(SocketHandler* loop, int error);
+  void PostOnError(SocketHandler* loop, int rv);
+  void OnError(int rv);
 
   // Only socket handlers which successfully bound and started are kept.
   ScopedVector<SocketHandler> socket_handlers_;
 
   Delegate* delegate_;
 
+  base::WeakPtrFactory<MDnsConnection> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(MDnsConnection);
 };
 

net/dns/mdns_client_unittest.cc

@@ -1093,6 +1093,8 @@ class MDnsConnectionTest : public ::testing::Test {
     socket_ipv6_ = new MockMDnsDatagramServerSocket(ADDRESS_FAMILY_IPV6);
     factory_.PushSocket(socket_ipv6_);
     factory_.PushSocket(socket_ipv4_);
+    sample_packet_ = MakeString(kSamplePacket1, sizeof(kSamplePacket1));
+    sample_buffer_ = new StringIOBuffer(sample_packet_);
   }
 
   bool InitConnection() {
@@ -1106,13 +1108,12 @@ class MDnsConnectionTest : public ::testing::Test {
   SimpleMockSocketFactory factory_;
   MDnsConnection connection_;
   TestCompletionCallback callback_;
+  std::string sample_packet_;
+  scoped_refptr<IOBuffer> sample_buffer_;
 };
 
 TEST_F(MDnsConnectionTest, ReceiveSynchronous) {
-  std::string sample_packet = MakeString(kSamplePacket1,
-      sizeof(kSamplePacket1));
-
-  socket_ipv6_->SetResponsePacket(sample_packet);
+  socket_ipv6_->SetResponsePacket(sample_packet_);
   EXPECT_CALL(*socket_ipv4_, RecvFrom(_, _, _, _))
       .WillOnce(Return(ERR_IO_PENDING));
   EXPECT_CALL(*socket_ipv6_, RecvFrom(_, _, _, _))
@@ -1120,64 +1121,103 @@ TEST_F(MDnsConnectionTest, ReceiveSynchronous) {
           Invoke(socket_ipv6_, &MockMDnsDatagramServerSocket::HandleRecvNow))
       .WillOnce(Return(ERR_IO_PENDING));
 
-  EXPECT_CALL(delegate_, HandlePacketInternal(sample_packet));
-
+  EXPECT_CALL(delegate_, HandlePacketInternal(sample_packet_));
   ASSERT_TRUE(InitConnection());
 }
 
 TEST_F(MDnsConnectionTest, ReceiveAsynchronous) {
-  std::string sample_packet = MakeString(kSamplePacket1,
-      sizeof(kSamplePacket1));
-  socket_ipv6_->SetResponsePacket(sample_packet);
+  socket_ipv6_->SetResponsePacket(sample_packet_);
+
   EXPECT_CALL(*socket_ipv4_, RecvFrom(_, _, _, _))
       .WillOnce(Return(ERR_IO_PENDING));
   EXPECT_CALL(*socket_ipv6_, RecvFrom(_, _, _, _))
+      .Times(2)
       .WillOnce(
-          Invoke(socket_ipv6_, &MockMDnsDatagramServerSocket::HandleRecvLater))
+           Invoke(socket_ipv6_, &MockMDnsDatagramServerSocket::HandleRecvLater))
       .WillOnce(Return(ERR_IO_PENDING));
 
   ASSERT_TRUE(InitConnection());
 
-  EXPECT_CALL(delegate_, HandlePacketInternal(sample_packet));
+  EXPECT_CALL(delegate_, HandlePacketInternal(sample_packet_));
 
   base::MessageLoop::current()->RunUntilIdle();
 }
 
-TEST_F(MDnsConnectionTest, Send) {
-  std::string sample_packet = MakeString(kSamplePacket1,
-      sizeof(kSamplePacket1));
-
-  scoped_refptr<IOBufferWithSize> buf(
-      new IOBufferWithSize(sizeof kSamplePacket1));
-  memcpy(buf->data(), kSamplePacket1, sizeof(kSamplePacket1));
+TEST_F(MDnsConnectionTest, Error) {
+  CompletionCallback callback;
 
   EXPECT_CALL(*socket_ipv4_, RecvFrom(_, _, _, _))
       .WillOnce(Return(ERR_IO_PENDING));
   EXPECT_CALL(*socket_ipv6_, RecvFrom(_, _, _, _))
-      .WillOnce(Return(ERR_IO_PENDING));
+      .WillOnce(DoAll(SaveArg<3>(&callback), Return(ERR_IO_PENDING)));
 
   ASSERT_TRUE(InitConnection());
 
+  EXPECT_CALL(delegate_, OnConnectionError(ERR_SOCKET_NOT_CONNECTED));
+  callback.Run(ERR_SOCKET_NOT_CONNECTED);
+  base::MessageLoop::current()->RunUntilIdle();
+}
+
+class MDnsConnectionSendTest : public MDnsConnectionTest {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    MDnsConnectionTest::SetUp();
+    EXPECT_CALL(*socket_ipv4_, RecvFrom(_, _, _, _))
+        .WillOnce(Return(ERR_IO_PENDING));
+    EXPECT_CALL(*socket_ipv6_, RecvFrom(_, _, _, _))
+        .WillOnce(Return(ERR_IO_PENDING));
+    EXPECT_TRUE(InitConnection());
+  }
+};
+
+TEST_F(MDnsConnectionSendTest, Send) {
   EXPECT_CALL(*socket_ipv4_,
-              SendToInternal(sample_packet, "224.0.0.251:5353", _));
+              SendToInternal(sample_packet_, "224.0.0.251:5353", _));
   EXPECT_CALL(*socket_ipv6_,
-              SendToInternal(sample_packet, "[ff02::fb]:5353", _));
+              SendToInternal(sample_packet_, "[ff02::fb]:5353", _));
 
-  connection_.Send(buf.get(), buf->size());
+  connection_.Send(sample_buffer_, sample_packet_.size());
 }
 
-TEST_F(MDnsConnectionTest, Error) {
+TEST_F(MDnsConnectionSendTest, SendError) {
   CompletionCallback callback;
 
-  EXPECT_CALL(*socket_ipv4_, RecvFrom(_, _, _, _))
-      .WillOnce(Return(ERR_IO_PENDING));
-  EXPECT_CALL(*socket_ipv6_, RecvFrom(_, _, _, _))
-      .WillOnce(DoAll(SaveArg<3>(&callback),  Return(ERR_IO_PENDING)));
-
-  ASSERT_TRUE(InitConnection());
+  EXPECT_CALL(*socket_ipv4_,
+              SendToInternal(sample_packet_, "224.0.0.251:5353", _));
+  EXPECT_CALL(*socket_ipv6_,
+              SendToInternal(sample_packet_, "[ff02::fb]:5353", _))
+      .WillOnce(DoAll(SaveArg<2>(&callback), Return(ERR_SOCKET_NOT_CONNECTED)));
 
+  connection_.Send(sample_buffer_, sample_packet_.size());
   EXPECT_CALL(delegate_, OnConnectionError(ERR_SOCKET_NOT_CONNECTED));
-  callback.Run(ERR_SOCKET_NOT_CONNECTED);
+  base::MessageLoop::current()->RunUntilIdle();
+}
+
+TEST_F(MDnsConnectionSendTest, SendQueued) {
+  // Send data immediately.
+  EXPECT_CALL(*socket_ipv4_,
+              SendToInternal(sample_packet_, "224.0.0.251:5353", _))
+      .Times(2)
+      .WillRepeatedly(Return(OK));
+
+  CompletionCallback callback;
+  // Delay sending data. Only the first call should be made.
+  EXPECT_CALL(*socket_ipv6_,
+              SendToInternal(sample_packet_, "[ff02::fb]:5353", _))
+      .WillOnce(DoAll(SaveArg<2>(&callback), Return(ERR_IO_PENDING)));
+
+  connection_.Send(sample_buffer_, sample_packet_.size());
+  connection_.Send(sample_buffer_, sample_packet_.size());
+
+  // The second IPv6 packed is not sent yet.
+  EXPECT_CALL(*socket_ipv4_,
+              SendToInternal(sample_packet_, "224.0.0.251:5353", _))
+      .Times(0);
+  // Expect call for the second IPv6 packed.
+  EXPECT_CALL(*socket_ipv6_,
+              SendToInternal(sample_packet_, "[ff02::fb]:5353", _))
+      .WillOnce(Return(OK));
+  callback.Run(OK);
 }
 
 }  // namespace