38 lines
1.9 KiB
Text
38 lines
1.9 KiB
Text
- There is way too much of busy waiting for fairly arbitrarily chosen
|
|
timeout periods in the code.
|
|
|
|
- The --clientport= option to a lookit process (when spawning them,
|
|
not forking) can not work as intended. The ClientPortNumber variable
|
|
is declared *static* in ChildProcessSession.hpp and thus is a
|
|
separate variable in each compilation unit (object file) that
|
|
includes ChildProcessSession.hpp. The variable that is assigned in
|
|
main() in LOOLBroker.cpp is not the variable used in
|
|
ChildProcessSession::downloadAs() in ChildProcessSession.cpp.
|
|
|
|
- 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...
|
|
|
|
- The use of separate "disconnect" messages over the WebSocket
|
|
connections is not good. It should be perfectly enough to just close
|
|
the WebSocket connection gracefully using the WebSocket mechanism,
|
|
i.e. a frame with the CLOSE opcode. Or just tearing down the socket
|
|
without a CLOSE frame. The code needs to be prepared to handle these
|
|
situations anyway, especially for the socket talking to the
|
|
client. For the internal communication in the process tree,
|
|
"disconnect" messages are barely acceptable, if the code is already
|
|
written to generate and expect them.
|
|
|
|
For the moment, we send them from the client too - to distinguish the
|
|
browser crash or connection loss from a deliberate shutdown.
|
|
We are saving the document in the case we don't get the explicit
|
|
'disconnect'. When we move to actually saving the document all
|
|
the time automatically, the 'disconnect' message should be removed.
|
|
|
|
- Occasionally Control-C (SIGINT) doesn't shut fown loolwsd. One has
|
|
to kill it with SIGKILL. Which of course leaves all the chroot jails
|
|
around.
|
|
|
|
- ASCII art? Seriously?
|