Skip to content
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions libstuff/SSocketPool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,31 @@ void SSocketPool::_timeoutThreadFunc()

unique_ptr<STCPManager::Socket> SSocketPool::getSocket()
{
{
// If there's an existing socket, return it.
lock_guard<mutex> lock(_poolMutex);
if (_sockets.size()) {
pair<chrono::steady_clock::time_point, unique_ptr<STCPManager::Socket>> s = move(_sockets.front());
while (true) {
unique_ptr<STCPManager::Socket> socket;
{
lock_guard<mutex> lock(_poolMutex);
if (_sockets.empty()) {
break;
}
socket = move(_sockets.front().second);
_sockets.pop_front();
return move(s.second);
}

// Verify the socket is still alive before returning it.
if (socket->state.load() == STCPManager::Socket::CONNECTED) {
Comment thread
tylerkaraszewski marked this conversation as resolved.
Outdated
struct pollfd pfd = {socket->s, POLLIN | POLLOUT, 0};
int ret = poll(&pfd, 1, 0);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Retry interrupted poll instead of discarding socket

If poll() is interrupted by a signal (ret == -1 with errno == EINTR), this path falls through to stale-socket discard, even though the socket may be perfectly valid. In environments with periodic signals (profilers, timers), this can cause avoidable connection churn and extra reconnect latency. Treat EINTR as retryable here rather than marking the socket dead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chances of this happening are so low and even if it happens the cost is just opening a new socket, I don't think this matters.

if (ret >= 0 && !(pfd.revents & (POLLERR | POLLHUP | POLLNVAL))) {
Comment on lines +66 to +68
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject peer-closed sockets before returning from pool

This liveness check treats a socket as healthy whenever poll() has no POLLERR|POLLHUP|POLLNVAL, but a graceful peer close is commonly reported as readable EOF (POLLIN, and POLLRDHUP only if requested) without POLLHUP. In that case this code returns a dead pooled socket, so callers still hit a failure on first I/O and the stale-socket issue remains. Please include POLLRDHUP in requested/checked events or perform an EOF probe (for example recv(..., MSG_PEEK)) before reusing the socket.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tylerkaraszewski is this valid?

return socket;
}
}

// Socket is dead, discard it (destructor closes fd).
SINFO("[SOCKET] Discarding stale socket from pool for host '" << host << "'.");
}

// If we get here, we need to create a socket to return. No need to hold the lock, so it goes out of scope.
// No live pooled socket available, create a new one. No need to hold the lock, so it goes out of scope.
try {
// TODO: Allow S_socket to take a parsed address instead of redoing all the parsing each time.
return unique_ptr<STCPManager::Socket>(new STCPManager::Socket(host));
Expand Down
Loading