Limit number of simultaneous REST connections

[tolikzinovyev]

Summary

This PR limits the number of simultaneous REST connections we process to prevent the exhaustion of resources and ultimately a crash.

Two limits are introduced: soft and hard. When the soft limit is exceeded, new connections are returned the 429 Too Many Requests http code. When the hard limit is exceeded, new connections are accepted and immediately closed.

Partially resolves https://github.com/algorand/go-algorand-internal/issues/1814.

Test Plan

Added unit tests.

[tsachiherman]

The HTTP connections are typically HTTP 1.1 connections. That makes them long-lasting connections, which won't quickly disconnect.

Limiting the number of concurrent connections to the number of CPU cores could be very problematic here. If the connections were CPU bounds, it would be just fine. But that's rarely the case. Some of the connection might be waiting for a block to arrive before returning.

I'd suggest moving the number of concurrent connection to be a config.json option, and supporting "-1" for disabling the connection limits.

[tsachiherman]

We need a unit test to confirm the code behaves as designed.
In this case, a unit test seems to be a good fit.

[winder]

How are the waiting connections queued by the operating system? It looks like the goroutine would be started and block in aquire while trying to write to the sem channel. This might work, and will prevent the SQLite db from being overloaded, but ideally we would reject requests with an error after the limit is reached.

[codecov-commenter]

Codecov Report

Merging #3326 (49f3f25) into master (8a335bb) will increase coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #3326      +/-   ##
==========================================
+ Coverage   47.67%   47.68%   +0.01%     
==========================================
  Files         369      370       +1     
  Lines       59660    59694      +34     
==========================================
+ Hits        28443    28466      +23     
- Misses      27906    27925      +19     
+ Partials     3311     3303       -8     

Impacted Files	Coverage Δ
config/localTemplate.go	50.00% <ø> (ø)
daemon/algod/api/server/router.go	15.15% <0.00%> (-1.52%)	⬇️
daemon/algod/server.go	5.19% <0.00%> (-0.92%)	⬇️
network/wsNetwork.go	62.98% <ø> (-0.06%)	⬇️
node/node.go	23.89% <ø> (ø)
network/limitlistener/rejectingLimitListener.go	88.88% <88.88%> (ø)
...od/api/server/lib/middlewares/connectionLimiter.go	91.66% <91.66%> (ø)
ledger/roundlru.go	90.56% <0.00%> (-5.67%)	⬇️
catchup/service.go	69.40% <0.00%> (-0.50%)	⬇️
network/wsPeer.go	68.05% <0.00%> (-0.28%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a335bb...49f3f25. Read the comment docs.

[tsachiherman]

How are the waiting connections queued by the operating system? It looks like the goroutine would be started and block in aquire while trying to write to the sem channel. This might work, and will prevent the SQLite db from being overloaded, but ideally we would reject requests with an error after the limit is reached.

The incoming connection here would reach the listening port (8080), and would be blocked there until Accept is being called. The other party would not receive a TCP ACK until we Accept the connection. When accepting the connection, a new local port would be allocated ( along with a file descriptor, naturally ), and at that point, we would be able to block the given connection. Since this is a HTTP connection, we could attempt to gracefully return "Server is busy" - but it's abit more tricky here, since we can't "write" on the accepting go-routine, since we don't want to block there.

[algorandskiy]

I agree this would work but

• num cpu does not look as a good limit
• I'd rather see a limiter that allows X concurrent requests and drops other with HTTP 429 rather accepting all and waiting in SYN SENT state.

Maybe consider using Echo middleware and handle limits on HTTP level rather TCP?

[tolikzinovyev]

The HTTP connections are typically HTTP 1.1 connections. That makes them long-lasting connections, which won't quickly disconnect.

Limiting the number of concurrent connections to the number of CPU cores could be very problematic here. If the connections were CPU bounds, it would be just fine. But that's rarely the case. Some of the connection might be waiting for a block to arrive before returning.

I'd suggest moving the number of concurrent connection to be a config.json option, and supporting "-1" for disabling the connection limits.

Makes sense!

[tolikzinovyev]

How are the waiting connections queued by the operating system? It looks like the goroutine would be started and block in aquire while trying to write to the sem channel. This might work, and will prevent the SQLite db from being overloaded, but ideally we would reject requests with an error after the limit is reached.

The incoming connection here would reach the listening port (8080), and would be blocked there until Accept is being called. The other party would not receive a TCP ACK until we Accept the connection. When accepting the connection, a new local port would be allocated ( along with a file descriptor, naturally ), and at that point, we would be able to block the given connection. Since this is a HTTP connection, we could attempt to gracefully return "Server is busy" - but it's abit more tricky here, since we can't "write" on the accepting go-routine, since we don't want to block there.

I think what I saw yesterday is that the OS establishes connections even when algod doesn't call accept() (netstat showed many ESTABLISHED connections). I can check again if that's important.

[tolikzinovyev]

I agree this would work but

* num cpu does not look as a good limit

* I'd rather see a limiter that allows X concurrent requests and drops other with HTTP 429 rather accepting all and waiting in SYN SENT state.

Maybe consider using Echo middleware and handle limits on HTTP level rather TCP?

I think this echo middleware limits the number of connections per second which is not what we want. In any case, go's http server creates a new goroutine for each accepted connections, so we must limit the number of connections in front of the http server.

[algorandskiy]

go's http server creates a new goroutine for each accepted connections, so we must limit the number of connections in front of the http server.

This is OK since they will be closed almost immediately with 429. I guess we'll run out socket descriptors faster than number of goroutines becomes a problem. That's why Echo simply defers new connection handing to the standard http.Server that creates new goroutine per request (note no one spawns a thread per request in production-ready http servers nowadays).

[winder]

I think this echo middleware limits the number of connections per second which is not what we want. In any case, go's http server creates a new goroutine for each accepted connections, so we must limit the number of connections in front of the http server.

That was my interpretation as well. The "burst" functionality was kind of neat, but also not relevant to the problem.

go's http server creates a new goroutine for each accepted connections, so we must limit the number of connections in front of the http server.

This is OK since they will be closed almost immediately with 429. I guess we'll run out socket descriptors faster than number of goroutines becomes a problem. That's why Echo simply defers new connection handing to the standard http.Server that creates new goroutine per request (note no one spawns a thread per request in production-ready http servers nowadays).

+1, I think that returning errors when the limit is reached should be ok.

[tolikzinovyev]

go's http server creates a new goroutine for each accepted connections, so we must limit the number of connections in front of the http server.

This is OK since they will be closed almost immediately with 429. I guess we'll run out socket descriptors faster than number of goroutines becomes a problem. That's why Echo simply defers new connection handing to the standard http.Server that creates new goroutine per request (note no one spawns a thread per request in production-ready http servers nowadays).

You are right that the number of goroutines is limited by the number of sockets. On linux its ~65000. With 4kb of memory per goroutine, that's about 260MB which is fine. I'm ok with it.

[tolikzinovyev]

go's http server creates a new goroutine for each accepted connections, so we must limit the number of connections in front of the http server.

This is OK since they will be closed almost immediately with 429. I guess we'll run out socket descriptors faster than number of goroutines becomes a problem. That's why Echo simply defers new connection handing to the standard http.Server that creates new goroutine per request (note no one spawns a thread per request in production-ready http servers nowadays).

You are right that the number of goroutines is limited by the number of sockets. On linux its ~65000. With 4kb of memory per goroutine, that's about 260MB which is fine. I'm ok with it.

On the other hand, not limiting the number of connections means other applications will not be able to make connections. That's pretty concerning.

[cce]

Agreed with comments here that a number in the thousands or 10s of thousands is a much better default than runtime.NumCPU(), or none at all (-1). But 10K seems like a reasonable default, as well, if you're just trying to come up with a value.

[tolikzinovyev]

go's http server creates a new goroutine for each accepted connections, so we must limit the number of connections in front of the http server.

This is OK since they will be closed almost immediately with 429. I guess we'll run out socket descriptors faster than number of goroutines becomes a problem. That's why Echo simply defers new connection handing to the standard http.Server that creates new goroutine per request (note no one spawns a thread per request in production-ready http servers nowadays).

I looked into your suggestion again. The number of file descriptors is basically unlimited. On my laptop sysctl fs.file-max returns a number > 10^9. So we do need a limit for the number of connections, at least to prevent memory exhaustion.

[algorandskiy]

This is kernel max for the entire system, not a single process, from https://www.kernel.org/doc/Documentation/sysctl/fs.txt

file-max & file-nr:

The value in file-max denotes the maximum number of file-
handles that the Linux kernel will allocate. When you get lots
of error messages about running out of file handles, you might
want to increase this limit.

[tolikzinovyev]

This is kernel max for the entire system, not a single process, from https://www.kernel.org/doc/Documentation/sysctl/fs.txt

file-max & file-nr:

The value in file-max denotes the maximum number of file-
handles that the Linux kernel will allocate. When you get lots
of error messages about running out of file handles, you might
want to increase this limit.

Right, there is also a per-process limit controlled by ulimit -n. On ubuntu and arch it defaults to 1024. Do you know if other linux/unix distros, osx and windows also have small limits?

[tsachiherman]

using these constants guarantee that your test would randomly fail on CI system, where the machine would randomly be super sluggish.
Instead of waiting for a given timeout, use a channel to "lock" this go-routine at this state. That should give you the ability to control the scheduling of each goroutine.

[tolikzinovyev]

I changed the test, but unfortunately, there is no way to completely avoid timing assumptions.

[tsachiherman]

@winder & @algorandskiy could you please confirm that all your requests around this PR have been met by approving it ?

[algorandskiy]

I have not got (yet) clarifications on why do we need that for{} loop in Accept()

[tolikzinovyev]

I have not got (yet) clarifications on why do we need that for{} loop in Accept()

I'm not sure what kind of clarification you are looking for. If you know a better way to implement this, please let me know.

[algorandskiy]

This looked weird for me because http.Serve already has a for-accept loop. But I see why the second for-accept loop is needed here as well.

[tsachiherman]

This looked weird for me because http.Serve already has a for-accept loop. But I see why the second for-accept loop is needed here as well.

I think that @tolikzinovyev is good; the http.Serve implementation is not well suited for a high throughput server. ( i.e. sleeping for 5ms is quite a long time when you want to support thousands of short-lived connections per second ).

[winder]

Thanks for the changes/explanation. This PR looks good!