halfgaar / FlashMQ

FlashMQ is a fast light-weight MQTT broker/server, designed to take good advantage of multi-CPU environments
https://www.flashmq.org/
Open Software License 3.0
173 stars 24 forks source link

Add option to set listener socket(s) to `TCP_NODELAY` mode #100

Closed bigsmoke closed 1 month ago

bigsmoke commented 1 month ago

How I came to the idea for this new option was when I learned that curl, since version 7.50.2, sets TCP_NODELAY by default.

TCP_NODELAY may decrease latency and may thus be worthwhile to be able to experiment with in some scenarios, to try if its benefits don't outweigh its main tradeoff (i.e., more, smaller TCP messages).

The reason that I added this as a boolean setting is that, as far as I can tell, there are only three modes to choose from, and I guessed that Wiebe is not interested in using TCP_CORK in the future:

  1. The default is that the TCP layer uses Nagle's algorithm to collect what would become multiple, small TCP packets into a larger packet until acknowledgement has been received for the last sent message.
  2. TCP_NODELAY sends out TCP packets immediately, possible reducing latency, but at the likely cost of efficiency.
  3. TCP_CORK does such buffering explicitly, waiting for setsockoption() from the application to flush the buffer.

If I am wrong and there is a potential future use for TCP_CORK, this option should probably be changed into an enum.

I have done a superficial test of this new feature using mosquitto_sub and mosquitto_pub, only to the extent to check if a listener with tcp_nodelay true still functions. What I did not do is to trace what actually happens on the TCP level.

I do not whether and where this needs to be added to the automated test suite.

halfgaar commented 1 month ago

Looks great :+1: . I have two requests:

1) I wasn't sure if TCP_NODELAY is inherited to the socket returned by accept() on the listener. I tested it, and it is, so that's good. Can the man page be extended with something like ''tcp_nodelay will cause the TCP_NODELAY option to be set for the listener's socket(s), and therefore all clients accepted on that listener"?

2) Outgoing bridges also need the option set, here. I think it can be a separate commit, this time for the option in a bridge section. Remember to add the option to this operator==(), to detect config change for hot-reloading bridges.

Tests are not required, I think. Although, it's not that hard to do now that I know TCP_CORK exists.

bigsmoke commented 1 month ago

By the way: the code style checker seems to have failed on a statement of which I did not change the style. Admittedly, that piece of code does not have the awesomest of styles. 🤷

halfgaar commented 1 month ago

It looks good to me. I'll squash that last commit and merge it soon.

halfgaar commented 1 month ago

I performed a few touch-ups and squashes, and merged it in.