BIND 10 trac2903, updated. 7d6c238e01f66ff90e073c458312e732b9ce3f72 [2903] updated coment about SyncUDPServer::answer_.
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon Apr 29 23:48:04 UTC 2013
The branch, trac2903 has been updated
via 7d6c238e01f66ff90e073c458312e732b9ce3f72 (commit)
via 719a1cb238f32588f5b891dc3f5d1461efff6905 (commit)
via 9f0934c0e2ca79fb5b1c03c57cc850fb9bdc22f8 (commit)
via 73bdf354a07a3e7ad4585881c434fd71c36ca775 (commit)
from 4933a1c6284498e2656909d15a84657b5b6ecb8b (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 7d6c238e01f66ff90e073c458312e732b9ce3f72
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Apr 29 16:28:43 2013 -0700
[2903] updated coment about SyncUDPServer::answer_.
commit 719a1cb238f32588f5b891dc3f5d1461efff6905
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Apr 29 16:00:57 2013 -0700
[2903] logged other errors from asio.
commit 9f0934c0e2ca79fb5b1c03c57cc850fb9bdc22f8
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Apr 29 14:39:32 2013 -0700
[2903] clarified one case after YIELD where the error shouldn't happen.
use assert() rather than ignoring it; we should be able to assert it safely
in this case.
commit 73bdf354a07a3e7ad4585881c434fd71c36ca775
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon Apr 29 14:05:44 2013 -0700
[2903] added more detailed explanation of when post() can throw
-----------------------------------------------------------------------
Summary of changes:
src/lib/asiodns/asiodns_messages.mes | 48 ++++++++++++++++++++++++++
src/lib/asiodns/sync_udp_server.cc | 12 ++++---
src/lib/asiodns/sync_udp_server.h | 14 ++++----
src/lib/asiodns/tcp_server.cc | 62 ++++++++++++++++++++++++----------
src/lib/asiodns/udp_server.cc | 6 ++--
5 files changed, 112 insertions(+), 30 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/asiodns/asiodns_messages.mes b/src/lib/asiodns/asiodns_messages.mes
index add1b04..bd2f81a 100644
--- a/src/lib/asiodns/asiodns_messages.mes
+++ b/src/lib/asiodns/asiodns_messages.mes
@@ -53,6 +53,11 @@ The asynchronous I/O code encountered an error when trying to send data to
the specified address on the given protocol. The number of the system
error that caused the problem is given in the message.
+% ASIODNS_SYNC_UDP_CLOSE_SOCKET_FAIL_ON_STOP failed to close a
+This is the same to ASIODNS_UDP_CLOSE_SOCKET_FAIL_ON_STOP but happens
+on the "synchronous UDP server", mainly used for the authoritative DNS
+server daemon.
+
% ASIODNS_TCP_ACCEPT_FAIL failed to accept TCP DNS connection: %1
Accepting a TCP connection from a DNS client failed due to an error
that could happen but should be rare. The reason for the error is
@@ -64,6 +69,44 @@ restrictive on this limitation, so consider adjusing the limit using
a tool such as ulimit. If you see other types of errors too often,
there may be something overlooked; please file a bug report in that case.
+% ASIODNS_TCP_CLOSE_ACCEPTOR_FAIL failed to close listening TCP socket: %1
+A TCP DNS server tried to close a listening TCP socket (for accepting
+new connections) as a step of stopping itself, but failed to do that.
+This is generally an unexpected event and so is logged as an error.
+
+% ASIODNS_TCP_CLOSE_FAIL failed to close DNS/TCP socket with a client: %1
+A TCP DNS server tried to close a TCP socket used to communicate with a
+client, but it failed to do that. This is expected to be very rare but
+we've seen it in real deployment.
+
+% ASIODNS_TCP_CLOSE_SOCKET_FAIL_ON_STOP failed to close a DNS/TCP socket while stopping the server: %1
+A TCP DNS server tried to close a TCP socket (which is either actively
+communicating with a client or is already unused) as a step of
+stopping itself, but failed to do that. This is generally an
+unexpected event and so is logged as an error.
+
+% ASIODNS_TCP_GETREMOTE_FAIL failed to get remote address of a DNS TCP connection: %1
+A TCP DNS server tried to get the address and port of a remote client
+on a connected socket but failed. It's expected to be rare but can
+still happen. See also ASIODNS_TCP_READLEN_FAIL.
+
+% ASIODNS_TCP_READDATA_FAIL failed to get DNS data on a TCP socket: %1
+A TCP DNS server tried to read a DNS message (that follows a 2-byte
+length field) but failed. It's expected to be rare but can still happen.
+See also ASIODNS_TCP_READLEN_FAIL.
+
+% ASIODNS_TCP_READLEN_FAIL failed to get DNS data length on a TCP socket: %1
+A TCP DNS server tried to get the length field of a DNS message (the first
+2 bytes of a new chunk of data) but failed. This is generally expected to
+be rare but can still happen, e.g, due to an unexpected reset of the
+connection. A specific reason for the failure is included in the log
+message.
+
+% ASIODNS_TCP_WRITE_FAIL failed to send DNS message over a TCP socket: %1
+A TCP DNS server tried to send a DNS message to a remote client but
+failed. It's expected to be rare but can still happen. See also
+ASIODNS_TCP_READLEN_FAIL.
+
% ASIODNS_UDP_ASYNC_SEND_FAIL Error sending UDP packet to %1: %2
The low-level ASIO library reported an error when trying to send a UDP
packet in asynchronous UDP mode. This can be any error reported by
@@ -75,6 +118,11 @@ If you see a single occurrence of this message, it probably does not
indicate any significant problem, but if it is logged often, it is probably
a good idea to inspect your network traffic.
+% ASIODNS_UDP_CLOSE_SOCKET_FAIL_ON_STOP failed to close a DNS/UDP socket while stopping the server: %1
+A UDP DNS server tried to close its UDP socket as a step of stopping
+itself, but failed to do that. This is generally an unexpected event
+and so is logged as an error.
+
% ASIODNS_UDP_RECEIVE_FAIL failed to receive UDP DNS packet: %1
Receiving a UDP packet from a DNS client failed due to an error that
could happen but should be very rare. The server still keeps
diff --git a/src/lib/asiodns/sync_udp_server.cc b/src/lib/asiodns/sync_udp_server.cc
index 3df3c25..aef6416 100644
--- a/src/lib/asiodns/sync_udp_server.cc
+++ b/src/lib/asiodns/sync_udp_server.cc
@@ -118,10 +118,10 @@ SyncUDPServer::handleRead(const asio::error_code& ec, const size_t length) {
// Good, there's an answer.
socket_->send_to(asio::const_buffers_1(output_buffer_->getData(),
output_buffer_->getLength()),
- sender_, 0, ecode_);
- if (ecode_) {
+ sender_, 0, ec_);
+ if (ec_) {
LOG_ERROR(logger, ASIODNS_UDP_SYNC_SEND_FAIL).
- arg(sender_.address().to_string()).arg(ecode_.message());
+ arg(sender_.address().to_string()).arg(ec_.message());
}
}
@@ -147,8 +147,12 @@ SyncUDPServer::stop() {
/// for it won't be scheduled by io service not matter it is
/// submit to io service before or after close call. And we will
/// get bad_descriptor error.
- socket_->close(ecode_); // pass error_code just to avoid getting exception.
+ socket_->close(ec_);
stopped_ = true;
+ if (ec_) {
+ LOG_ERROR(logger, ASIODNS_SYNC_UDP_CLOSE_SOCKET_FAIL_ON_STOP).
+ arg(ec_.message());
+ }
}
void
diff --git a/src/lib/asiodns/sync_udp_server.h b/src/lib/asiodns/sync_udp_server.h
index 77b9c14..ab33e7e 100644
--- a/src/lib/asiodns/sync_udp_server.h
+++ b/src/lib/asiodns/sync_udp_server.h
@@ -52,10 +52,10 @@ public:
/// callback is NULL. So the constructor explicitly rejects that case
/// with an exception. Likewise, it doesn't take "checkin" or "answer"
/// callbacks. In fact, calling "checkin" from receive callback does not
- /// make sense for any of the DNSServer variants; "answer" callback is
- /// simply unnecessary for this class because a complete answer is build
- /// in the lookup callback (it's the user's responsibility to guarantee
- /// that condition).
+ /// make sense for any of the DNSServer variants (see Trac #2935);
+ /// "answer" callback is simply unnecessary for this class because a
+ /// complete answer is built in the lookup callback (it's the user's
+ /// responsibility to guarantee that condition).
///
/// \param io_service the asio::io_service to work with
/// \param fd the file descriptor of opened UDP socket
@@ -128,7 +128,9 @@ private:
// If it was OK to have just a buffer, not the wrapper class,
// we could reuse the data_
isc::util::OutputBufferPtr output_buffer_;
- // Objects to hold the query message and the answer: these are not used
+ // Objects to hold the query message and the answer. The latter isn't
+ // used and only defined as a placeholder as the callback signature
+ // requires it.
isc::dns::MessagePtr query_, answer_;
// The socket used for the communication
std::auto_ptr<asio::ip::udp::socket> socket_;
@@ -153,7 +155,7 @@ private:
bool stopped_;
// Placeholder for error code object. It will be passed to ASIO library
// to have it set in case of error.
- asio::error_code ecode_;
+ asio::error_code ec_;
// The callback functor for internal asynchronous read event. This is
// stateless (and it will be copied in the ASIO library anyway), so
// can be const
diff --git a/src/lib/asiodns/tcp_server.cc b/src/lib/asiodns/tcp_server.cc
index a3b9d85..5f2b08a 100644
--- a/src/lib/asiodns/tcp_server.cc
+++ b/src/lib/asiodns/tcp_server.cc
@@ -14,13 +14,6 @@
#include <config.h>
-#include <unistd.h> // for some IPC/network system calls
-#include <netinet/in.h>
-#include <sys/socket.h>
-#include <errno.h>
-
-#include <boost/shared_array.hpp>
-
#include <log/dummylog.h>
#include <util/buffer.h>
@@ -32,6 +25,14 @@
#include <asiodns/tcp_server.h>
#include <asiodns/logger.h>
+#include <boost/shared_array.hpp>
+
+#include <cassert>
+#include <unistd.h> // for some IPC/network system calls
+#include <netinet/in.h>
+#include <sys/socket.h>
+#include <errno.h>
+
using namespace asio;
using asio::ip::udp;
using asio::ip::tcp;
@@ -161,6 +162,8 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
CORO_YIELD async_read(*socket_, asio::buffer(data_.get(),
TCP_MESSAGE_LENGTHSIZE), *this);
if (ec) {
+ LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_READLEN_FAIL).
+ arg(ec.message());
return;
}
@@ -168,11 +171,12 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
/// to allow inline variable declarations.)
CORO_YIELD {
InputBuffer dnsbuffer(data_.get(), length);
- uint16_t msglen = dnsbuffer.readUint16();
+ const uint16_t msglen = dnsbuffer.readUint16();
async_read(*socket_, asio::buffer(data_.get(), msglen), *this);
}
-
if (ec) {
+ LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_READDATA_FAIL).
+ arg(ec.message());
return;
}
@@ -183,6 +187,8 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
// all these calls to "new".)
peer_.reset(new TCPEndpoint(socket_->remote_endpoint(ec)));
if (ec) {
+ LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_GETREMOTE_FAIL).
+ arg(ec.message());
return;
}
@@ -220,11 +226,11 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
// Schedule a DNS lookup, and yield. When the lookup is
// finished, the coroutine will resume immediately after
- // this point.
+ // this point. On resume, this method should be called with its
+ // default parameter values (because of the signature of post()'s
+ // handler), so ec shouldn't indicate any error.
CORO_YIELD io_.post(AsyncLookup<TCPServer>(*this));
- if (ec) {
- return;
- }
+ assert(!ec);
// The 'done_' flag indicates whether we have an answer
// to send back. If not, exit the coroutine permanently.
@@ -249,13 +255,23 @@ TCPServer::operator()(asio::error_code ec, size_t length) {
// (though we have nothing further to do, so the coroutine
// will simply exit at that time).
CORO_YIELD async_write(*socket_, bufs, *this);
+ if (ec) {
+ LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_WRITE_FAIL).
+ arg(ec.message());
+ }
// All done, cancel the timeout timer. if it throws, consider it fatal.
timeout_->cancel();
// TODO: should we keep the connection open for a short time
// to see if new requests come in?
- socket_->close(ec); // ignore any error on close()
+ socket_->close(ec);
+ if (ec) {
+ // close() should be unlikely to fail, but we've seen it fail once,
+ // so we log the event.
+ LOG_DEBUG(logger, DBGLVL_TRACE_BASIC, ASIODNS_TCP_CLOSE_FAIL).
+ arg(ec.message());
+ }
}
}
@@ -268,18 +284,24 @@ TCPServer::asyncLookup() {
}
void TCPServer::stop() {
- // passing error_code to avoid getting exception; we simply ignore any
- // error on close().
asio::error_code ec;
/// we use close instead of cancel, with the same reason
/// with udp server stop, refer to the udp server code
acceptor_->close(ec);
+ if (ec) {
+ LOG_ERROR(logger, ASIODNS_TCP_CLOSE_ACCEPTOR_FAIL).arg(ec.message());
+ }
+
// User may stop the server even when it hasn't started to
- // run, in that that socket_ is empty
+ // run, in that case socket_ is empty
if (socket_) {
socket_->close(ec);
+ if (ec) {
+ LOG_ERROR(logger, ASIODNS_TCP_CLOSE_SOCKET_FAIL_ON_STOP).
+ arg(ec.message());
+ }
}
}
/// Post this coroutine on the ASIO service queue so that it will
@@ -288,7 +310,11 @@ void TCPServer::stop() {
void
TCPServer::resume(const bool done) {
done_ = done;
- io_.post(*this); // this can throw, but can be considered fatal.
+
+ // post() can throw due to memory allocation failure, but as like other
+ // cases of the entire BIND 10 implementation, we consider it fatal and
+ // let the exception be propagated.
+ io_.post(*this);
}
} // namespace asiodns
diff --git a/src/lib/asiodns/udp_server.cc b/src/lib/asiodns/udp_server.cc
index dbb2008..2a3e3b1 100644
--- a/src/lib/asiodns/udp_server.cc
+++ b/src/lib/asiodns/udp_server.cc
@@ -327,8 +327,6 @@ UDPServer::asyncLookup() {
/// Stop the UDPServer
void
UDPServer::stop() {
- // passing error_code to avoid getting exception; we simply ignore any
- // error on close().
asio::error_code ec;
/// Using close instead of cancel, because cancel
@@ -340,6 +338,10 @@ UDPServer::stop() {
/// submit to io service before or after close call. And we will
// get bad_descriptor error.
data_->socket_->close(ec);
+ if (ec) {
+ LOG_ERROR(logger, ASIODNS_UDP_CLOSE_SOCKET_FAIL_ON_STOP).
+ arg(ec.message());
+ }
}
/// Post this coroutine on the ASIO service queue so that it will
More information about the bind10-changes
mailing list