51 lines
2.3 KiB
Text
51 lines
2.3 KiB
Text
- 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. The last remaining instances should be
|
|
removed from the Kit/ChildSession.cppp as/when we move to a single
|
|
threaded Kit process.
|
|
|
|
- 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.
|