Skip to content

Commit

Permalink
Apply RAII to UnixSocket
Browse files Browse the repository at this point in the history
This deletes the `UnixSocket` copy constructors and adds move constructors and
a destructor. The `NetAccept` and `NetAcceptEvent` classes now store shared
pointers to their server instances because of this change.
  • Loading branch information
JosiahWI committed Oct 12, 2024
1 parent ffa0b26 commit 900418d
Show file tree
Hide file tree
Showing 13 changed files with 101 additions and 76 deletions.
8 changes: 8 additions & 0 deletions include/iocore/eventsystem/UnixSocket.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ class UnixSocket
*/
UnixSocket(int domain, int ctype, int protocol);

~UnixSocket();

UnixSocket(UnixSocket const &other) = delete;
UnixSocket &operator=(UnixSocket const &other) = delete;

UnixSocket(UnixSocket &&other);
UnixSocket &operator=(UnixSocket &&other);

int get_fd() const;

bool is_ok() const;
Expand Down
6 changes: 3 additions & 3 deletions src/iocore/dns/DNS.cc
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ DNSHandler::retry_named(int ndx, ink_hrtime t, bool reopen)
open_cons(&m_res->nsaddr_list[ndx].sa, true, ndx);
}
bool over_tcp = dns_conn_mode == DNS_CONN_MODE::TCP_ONLY;
UnixSocket con_sock = over_tcp ? tcpcon[ndx].sock : udpcon[ndx].sock;
UnixSocket &con_sock = over_tcp ? tcpcon[ndx].sock : udpcon[ndx].sock;
unsigned char buffer[MAX_DNS_REQUEST_LEN];
Dbg(dbg_ctl_dns, "trying to resolve '%s' from DNS connection, ndx %d", try_server_names[try_servers], ndx);
int r = _ink_res_mkquery(m_res, try_server_names[try_servers], T_A, buffer, over_tcp);
Expand All @@ -703,7 +703,7 @@ DNSHandler::try_primary_named(bool reopen)
if ((t - last_primary_retry) > DNS_PRIMARY_RETRY_PERIOD) {
unsigned char buffer[MAX_DNS_REQUEST_LEN];
bool over_tcp = dns_conn_mode == DNS_CONN_MODE::TCP_ONLY;
UnixSocket con_sock = over_tcp ? tcpcon[0].sock : udpcon[0].sock;
UnixSocket &con_sock = over_tcp ? tcpcon[0].sock : udpcon[0].sock;
last_primary_retry = t;
Dbg(dbg_ctl_dns, "trying to resolve '%s' from primary DNS connection", try_server_names[try_servers]);
int r = _ink_res_mkquery(m_res, try_server_names[try_servers], T_A, buffer, over_tcp);
Expand Down Expand Up @@ -1183,7 +1183,7 @@ write_dns_event(DNSHandler *h, DNSEntry *e, bool over_tcp)
h->release_query_id(e->id[dns_retries - e->retries]);
}
e->id[dns_retries - e->retries] = i;
UnixSocket con_sock = over_tcp ? h->tcpcon[h->name_server].sock : h->udpcon[h->name_server].sock;
UnixSocket &con_sock = over_tcp ? h->tcpcon[h->name_server].sock : h->udpcon[h->name_server].sock;
Dbg(dbg_ctl_dns, "send query (qtype=%d) for %s to fd %d", e->qtype, e->qname, con_sock.get_fd());

int s = con_sock.send(buffer, r, 0);
Expand Down
21 changes: 21 additions & 0 deletions src/iocore/eventsystem/UnixSocket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,27 @@ static int accept4(int sockfd, struct sockaddr *addr, socklen_t *addrlen, int fl
#endif
static unsigned int read_uint_from_fd(int fd);

UnixSocket::UnixSocket(UnixSocket &&other)
{
this->fd = other.fd;
other.fd = NO_SOCK;
}

UnixSocket::~UnixSocket()
{
if (this->is_ok()) {
this->close();
}
}

UnixSocket &
UnixSocket::operator=(UnixSocket &&other)
{
this->fd = other.fd;
other.fd = NO_SOCK;
return *this;
}

int
UnixSocket::set_nonblocking()
{
Expand Down
8 changes: 3 additions & 5 deletions src/iocore/net/Connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ Connection::move(Connection &orig)
{
this->is_connected = orig.is_connected;
this->is_bound = orig.is_bound;
this->sock = orig.sock;
// The target has taken ownership of the file descriptor
orig.sock = UnixSocket{NO_FD};
this->addr = orig.addr;
this->sock_type = orig.sock_type;
this->sock = std::move(orig.sock);
this->addr = orig.addr;
this->sock_type = orig.sock_type;
}
2 changes: 1 addition & 1 deletion src/iocore/net/NetAcceptEventIO.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ int
NetAcceptEventIO::start(EventLoop l, NetAccept *na, int events)
{
_na = na;
return start_common(l, _na->server.sock.get_fd(), events);
return start_common(l, _na->server->sock.get_fd(), events);
}
void
NetAcceptEventIO::process_event(int /* flags ATS_UNUSED */)
Expand Down
16 changes: 4 additions & 12 deletions src/iocore/net/P_Connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ struct Connection {

virtual ~Connection();
Connection();
Connection(Connection const &that) = delete;
Connection(Connection const &that) = delete;
Connection &operator=(Connection const &that) = delete;

/// Default options.
static NetVCOptions const DEFAULT_OPTIONS;
Expand All @@ -119,15 +120,6 @@ struct Connection {
*/
void move(Connection &);

protected:
/** Assignment operator.
*
* @param that Source object.
* @return @a this
*
* This is protected because it is not safe in the general case, but is valid for
* certain subclasses. Those provide a public assignment that depends on this method.
*/
Connection &operator=(Connection const &that) = default;
void _cleanup();
private:
void _cleanup();
};
22 changes: 12 additions & 10 deletions src/iocore/net/P_NetAccept.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,11 @@
#include "iocore/net/NetAcceptEventIO.h"
#include "Server.h"

#include <vector>
#include "tscore/ink_platform.h"

#include <memory>
#include <vector>

struct NetAccept;
struct HttpProxyPort;
class Event;
Expand All @@ -62,7 +64,7 @@ class UnixNetVConnection;

// TODO fix race between cancel accept and call back
struct NetAcceptAction : public Action, public RefCountObjInHeap {
Server *server;
std::shared_ptr<Server> server;

void
cancel(Continuation *cont = nullptr) override
Expand All @@ -89,14 +91,14 @@ struct NetAcceptAction : public Action, public RefCountObjInHeap {
// Handles accepting connections.
//
struct NetAccept : public Continuation {
ink_hrtime period = 0;
Server server;
AcceptFunctionPtr accept_fn = nullptr;
int ifd = NO_FD;
int id = -1;
Ptr<NetAcceptAction> action_;
SSLNextProtocolAccept *snpa = nullptr;
NetAcceptEventIO ep;
ink_hrtime period = 0;
std::shared_ptr<Server> server;
AcceptFunctionPtr accept_fn = nullptr;
int ifd = NO_FD;
int id = -1;
Ptr<NetAcceptAction> action_;
SSLNextProtocolAccept *snpa = nullptr;
NetAcceptEventIO ep;

HttpProxyPort *proxyPort = nullptr;
AcceptOptions opt;
Expand Down
8 changes: 4 additions & 4 deletions src/iocore/net/QUICNetProcessor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -250,13 +250,13 @@ QUICNetProcessor::main_accept(Continuation *cont, SOCKET fd, AcceptOptions const
ink_assert(0 < opt.local_port && opt.local_port < 65536);
accept_ip.network_order_port() = htons(opt.local_port);

na->accept_fn = net_accept;
na->server.sock = UnixSocket{fd};
ats_ip_copy(&na->server.accept_addr, &accept_ip);
na->accept_fn = net_accept;
na->server->sock = UnixSocket{fd};
ats_ip_copy(&na->server->accept_addr, &accept_ip);

na->action_ = new NetAcceptAction();
*na->action_ = cont;
na->action_->server = &na->server;
na->action_->server = na->server;
na->init_accept();

return na->action_.get();
Expand Down
2 changes: 1 addition & 1 deletion src/iocore/net/QUICPacketHandler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ QUICPacketHandlerIn::acceptEvent(int event, void *data)
} else if (event == EVENT_IMMEDIATE) {
this->setThreadAffinity(this_ethread());
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
udpNet.UDPBind((Continuation *)this, &this->server.accept_addr.sa, -1, 1048576, 1048576);
udpNet.UDPBind((Continuation *)this, &this->server->accept_addr.sa, -1, 1048576, 1048576);
return EVENT_CONT;
}

Expand Down
2 changes: 2 additions & 0 deletions src/iocore/net/Server.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
#include "tscore/ink_inet.h"
#include "tscore/ink_memory.h"

#include <memory>

struct Server {
/// Client side (inbound) local IP address.
IpEndpoint accept_addr;
Expand Down
Loading

0 comments on commit 900418d

Please sign in to comment.