- The URL of the document to view and/or edit is specified both in the HTTP GET and in the 'load' message. This is redundant, and adds completely unnecessary, hard to define, and hard to test, states that the server and clients can be in. For instance: - Is it an error to send a different URL in the 'load' message than the one in the GET? (Of course it is. Do we check that? Do we have a unit test to make sure it stays an error?) - What messages are accepted by the server before the 'load'? What messages are sent to the client? Do they make sense, if it is only the 'load' that actually fully completes the association of a session with a document? Do we have unit tests for this? - What if one client connects, and then before it has sent the 'load' message, another client connects specifying the same document in the GET, and also sends a 'load' message. Which client gets the edit lock? Is that arbitrary or specified? Do we have a unit test for it? - Etc - There is way too much of busy waiting for fairly arbitrarily chosen timeout periods in the code. With "busy wait" here I of course don't mean *real* busy wait that would actually mindlessly spin the CPU. I mean calling poll() with some short timeout (order of seconds), and each time a timeout happens (which surely is the large majority of times), check a condition, which in the large majority of times will not be true, and then repeat. Instead the code could use for instance eventfds to enable the poll() calls to be without timeout. Or something similar, depending on case. - Recursive mutexes are evil. In general, I think the consensus is that recursive mutexes should be avoided. One should use them only when absolutely necessary because the code-base is so complex that one has no idea how it works. That was hopefully not the case when recursive mutexes were introduced here? But probably it is by now... - Occasionally Control-C (SIGINT) doesn't shut down loolwsd. One has to kill it with SIGKILL. Which of course leaves all the chroot jails around. - There are lots of places where a std::string variable is defined, initialised with a value, that is never changed. (In many cases it is const, so could of course not be changed.) Then the variable is used just once or twice, passed as a parameter to a function, or used in a comparison expression. This is fairly pointless and just makes the code harder to read. Use string literals instead.