1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
|
- 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.
|
