Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"max socket lifetime" Agent impacted by v13.11 socket timeout changes #33111

Closed
omsmith opened this issue Apr 28, 2020 · 2 comments
Closed

"max socket lifetime" Agent impacted by v13.11 socket timeout changes #33111

omsmith opened this issue Apr 28, 2020 · 2 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@omsmith
Copy link
Contributor

omsmith commented Apr 28, 2020

  • Version: v13.11.0+
  • Platform:
  • Subsystem: http

We have a desire to limit the overall lifetime of a socket used for http keep-alive, we've done this with a simple (possibly non-ideal) agent implementation that looks similar to below:

Expand to see HttpAgent implementation
'use strict';

const _HttpsAgent = require('https').Agent;

const MAX_SOCKET_LIFETIME_MS = 10 * 60 * 1000;
const MAX_KEEPALIVE_IDLE_MS = 50 * 1000;

// By default timeouts on a socket don't really do anything. As such, after
// setting a timeout when a socket is kept for keep-alive, it will still get
// re-used again after the timeout period unless it is closed by other means.
// Listening for the timeout event lets use close the socket and remove it from
// the pool. This listener is added when sockets get kept, and removed when
// they get re-used, so should have no risk of effecting external behaviour.
function onTimeout() {
	this.destroy();
}

function setSocketCreationTime(err, socket) {
	if (err) return;

	socket._creation = Date.now();
}

function socketIsExpired(agent, socket) {
	if (!socket._creation) {
		// shouldn't happen, but covering my bases
		setSocketCreationTime(null, socket);
		return false;
	}

	return Date.now() - socket._creation > agent.maxSocketLifetime;
}

function setSocketTimeout(agent, socket) {
	const ttl = socket._creation - Date.now() + agent.maxSocketLifetime;
	const timeout = Math.min(ttl, agent.maxIdleTimeout);
	socket.setTimeout(timeout, onTimeout);
}

function clearSocketTimeout(socket) {
	socket.setTimeout(0, onTimeout);
}

class HttpsAgent extends _HttpsAgent {
	constructor(opts) {
		opts = opts || {};
		opts.keepAlive = opts.keepAlive !== false;

		super(opts);

		this.maxSocketLifetime = MAX_SOCKET_LIFETIME_MS;
		this.maxIdleTimeout = MAX_KEEPALIVE_IDLE_MS;
	}

	createSocket(req, options, cb) {
		return super.createSocket(req, options, (err, socket) => {
			setSocketCreationTime(err, socket);
			return cb(err, socket);
		});
	}

	keepSocketAlive(socket) {
		// while we want keep-alive for latency/performance, we don't want to
		// keep sockets around forever so that if a backend moves (without
		// necessarily closing connections) we will eventually hit the new
		// addresses
		if (socketIsExpired(this, socket)) {
			// returning false means the agent will destroy the socket instead
			// of keeping it for re-use
			return false;
		}

		// by default sockets will never time out
		// set a timeout for our remaining desired lifetime of the socket, so
		// if it doesn't get re-used before then, it will be destroyed for
		// being too old while sitting idle
		setSocketTimeout(this, socket);
		return super.keepSocketAlive(socket);
	}

	reuseSocket(socket, req) {
		// we set a timeout when this socket last went back into the pool so
		// that it didnt sit idle past its desired lifetime; however that could
		// have been as little as say, 100ms. We don't want to cause
		// inconsistent behaviour, having the socket time out during a "slow"
		// request to the backend, so reset the timeout back to infinity.
		// this can still be set by request.timeout() afterward, as normal
		clearSocketTimeout(socket);
		return super.reuseSocket(socket, req);
	}
}

The overall approach:

  • Track when the socket was created
  • Check if its passed its lifetime when entering the freelist and reject it
  • Set a timeout for its remaining lifetime (or a "max free time") when entering the freelist

Which has served us fairly well, and I believe similar mechanisms exist in more featureful "keep-alive agent" node modules.

The built-in agent started trying to handle some of this behaviour in 13.11 (8700d89#diff-5f7fb0850412c6be189faeddea6c5359 #32000) by setting its own timeout listener (removing the need for our agent's onTimeout), but also by resetting the socket's timeout just before re-inserting it into the freelist. Unfortunately this setTimeout proceeds the call to keepSocketAlive, overriding the behaviour we've implemented.

        } else if (this.keepSocketAlive(socket)) {
          freeSockets = freeSockets || [];
          this.freeSockets[name] = freeSockets;
          socket[async_id_symbol] = -1;
          socket._httpMessage = null;
          this.removeSocket(socket, options);

+          const agentTimeout = this.options.timeout || 0;
+          if (socket.timeout !== agentTimeout) {
+            socket.setTimeout(agentTimeout);
+          }
+
          freeSockets.push(socket);

The documentation on the agent's overridable hooks doesn't necessarily describe what may happen before or after them, but this is something we'd like to be able to maintain.

I may be able to follow up with a PR in the next few days. My first thought would be to move the agent's built-in timeout reset be moved into the default keepSocketAlive function.

Thanks!

@lpinca
Copy link
Member

lpinca commented Apr 28, 2020

cc: @ronag

@lpinca lpinca added the http Issues or PRs related to the http subsystem. label Apr 28, 2020
@ronag
Copy link
Member

ronag commented Apr 28, 2020

So basically we should move:

+          const agentTimeout = this.options.timeout || 0;
+          if (socket.timeout !== agentTimeout) {
+            socket.setTimeout(agentTimeout);
+          }

into the default keepSocketAlive so that you can override the behavior. Sounds reasonable. Would you mind opening a PR?

@ronag ronag closed this as completed in 0413acc Apr 30, 2020
targos pushed a commit that referenced this issue May 4, 2020
Previous location of setting the timeout would override
behaviour of custom HttpAgents' keepSocketAlive. Moving
it into the default keepSocketAlive allows it to
interoperate with custom agents.

Fixes: #33111

PR-URL: #33127
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
ronag pushed a commit to nxtedition/node that referenced this issue May 10, 2020
Previous location of setting the timeout would override
behaviour of custom HttpAgents' keepSocketAlive. Moving
it into the default keepSocketAlive allows it to
interoperate with custom agents.

Fixes: nodejs#33111

PR-URL: nodejs#33127
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
Previous location of setting the timeout would override behaviour of custom HttpAgents' keepSocketAlive. Moving it into the default keepSocketAlive allows it to interoperate with custom agents.

Fixes: nodejs/node#33111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants