Remove locks and replace with isCorrectThread
assertions instead.
Crash recovery still needs some work, but
otherwise tests are clean (91/94 pass).
Change-Id: I9ac3e21854447d19a8e6106487dfd8be00fcf5ef
Since this doesn't necessary mean the document
was loaded completely (as the similarly named
flag in DocumentBroker signifies) rather that
the session was added to DocumentBroker.
Change-Id: Ibfc702bbd111ade2715dcb28ac3aa4e9e8e025dd
Sessions are now added to the DocBroker
_sessions map and loaded from the poll
thread first before processing their
messages.
Since messages are not read from the
sockets outside of the poll thread,
there is no reason to queue them
at all. The only exception is when
messages are passed directly
to ClientSession during convert-to
requests. That will be handled
separately (for now convert-to test
fails).
Change-Id: I798166b9e45b5707a33d31137e01a32ce63630b1
Reviewed-on: https://gerrit.libreoffice.org/35705
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Termination should normally be initiated by the
DocumentBroker in question, so sending of termination
message on the sockets come from the correct thread.
When termination happens from elsewhere
(f.e. cleanupDocBrokers) we cannot send socket
messages, and have to resort to rude termination.
Change-Id: I94acb7b314f5dbdc45c57049fc1ac8527ba72fb9
While message emplacement happens in the DocumentBroker poll, we
can be sure that the next iteration of the poll will call
hasQueuedWrites before polling.
This reverts commit 388d7b1dbf.
It is vital to have clean control of thread start. By starting
a thread during init. of a member (or base-clase) we loose lots of
control, some examples:
Any members initialized after a member that auto-starts a
thread, is effectively un-defined, and cannot be accessed
reliably.
This is particularly problematic for sub-classes.
I've seen threads started before the base-class has
finished constructing in the original creating thread -
such that the vtable was not yet updated to the sub-class
causing the transient parent vtable used during construction
to be used in-error.
Since all TerminatingPoll instances need to fire
a thread, no reason to do it manually and risk
races. Now TerminatingPoll does it in the ctor.
Change-Id: I59850ad48b3789f3a23d01abb05a7f28e5717031
Reviewed-on: https://gerrit.libreoffice.org/35114
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
DocumentBrokerPoll is always owned by a
single DocumentBroker instance, so we
can hold a reference to it. This eliminates
the need to hold a shared_ptr to the owner
which, in turn, eliminates the need for
a create wrapper.
Change-Id: I954c9dddcc3b2cfdd5dfcc8248ab3d47a897f684
Reviewed-on: https://gerrit.libreoffice.org/35113
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
When the last client session is disconnected
docBroker must first issue a save and wait
until the kit processes the save and sends
back notfication. Since said notification
goes to the ChildSession (which is the last)
and said ChildSession is the one that signals
to docBroker to persist the doc to the Storage,
we need to keep all components alive and kicking
during this final saving.
As such, when the last session is to be removed
from docBroker, we instead issue an autosave and
continue everything as normal. When the save
notification even arrives and ChildSession signals
docBroker to persist the doc, we check if we were
destroying and in that even remove that last session
and stop the polling thread.
The docBroker instance itself will get cleaned up
in due time.
Change-Id: Ie84e784284e1ec12b0b201d6bf75170b31f66147
Without this, it was impossible to connect to an existing session, because we
were trying to send messages to sessions that were not connected yet.
Change-Id: If9260a1f0ac8f5387f492541548724b0065df9d9
Don't block when creating new sessions, instead queue the requests, and handle
them in the DocumentBroker.
Change-Id: I200bbfc740f004c37178fa316d1eb91afdde5d4a
Also dung out a chunk of older code.
FIXME: websocket / ClientSession needs to associate itself with the
DocumentBroker poll loop in place of the original loop.
When cleaning up DocumentBrokers we hold
the global DocBrokersMutex. So we need
to keep this lock as short as possible
to serve new requests.
However when a given DocBroker is locked
and busy, or worse deadlocked, we'd end
up blocking any new client connection.
So here we skip busy DocBrokers while
cleaning up.
Change-Id: I188c9abc34fd90c4ba388894b47c1ab08d185129
Reviewed-on: https://gerrit.libreoffice.org/34119
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
By default snapshots are disabled, since trace recording
is enabled, to avoid unexpectedly flooding the disk.
Change-Id: I6c8728e14801f0a72accde1378455ec0e6046e3e
We no longer tell the clinet "This is embarrassing..."
when we disconnect and unload an idle document. Instead,
the client UI remains greyed out so the user can resume
as if it was inactive (and reload the document in this case).
Also, we now always send the "close: " message prior
to shutting down a client websocket. This is more
reasonable and consistent when we intentionally disconnect,
so clients can rely on it to signal intent and give reason.
Otherwise, a disconnection without this application-level
message should be unexpected and is therefore reasonable
to show the "This is embarrassing..." message.
Change-Id: Ic7439bcc9267be155586ccd5d122e9fe60225516
A race condition between the client socket thread
and the idle-document cleanup caused segfault
on the websocket.
Now the ChildProcess object doesn't reset
the websocket on closing, rather on destruction.
Change-Id: I10d0dfb1ba677a65479df85b7a53de8c5f1b44c3
When saving we need to differentiate between no-op
and failure. The lastSaveTime must always be updated
when saving didn't fail (i.e. no modification or saved).
Change-Id: I0e2455afac22c82f0b623f9441fbc0bca8a7cb83
Reviewed-on: https://gerrit.libreoffice.org/32629
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
While time_t is much simpler, it's too
opaque. The new chrono library is type-safe
and does conversion correctly, as well as
guarantees monotonity and other desirable
properties.
Change-Id: Id41c44c397a31d73e894e8f1715ff18f2b67df53
Reviewed-on: https://gerrit.libreoffice.org/32627
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
The WS could be chocked on write, but we shouldn't
assume the child is dead because of that. We are
only trying to test if the child process is alive
in that helper.
Change-Id: I108a297e43f923cab0dfa30204837dc4df15d3a1
Reviewed-on: https://gerrit.libreoffice.org/32289
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
For now, do the check only when a new session connects to the
document, because at that point we fetch the document information (in
separate function for WOPI and local files) and construct the
FileInfo, including timestamp.
For now, just log an ERR message if we notice that the document in its
storage system (WOPI or local file system) has an unexpected last
modified time. What should we do? If we don't have unsaved changes,
most likely we should just silently reload the document and force all
sessions to refresh. But if we have unsaved changes, and the document
has changed underneath, we have a problem.
We need to fetch the timestamp also also after saving ("persisting")
as we can't assume that the clock on the machine running loolwsd and
that of the storage (as reported in the WOPI case in CheckFileInfo)
are in synch. (Assuming separate machines, they certainly won't ever
exactly in synch, but aren't necessarily even just a few seconds apart
(think incorrectly set up timezone etc), so no amount of tolerance in
the comparison would be good enough, because after all, it might be
that in the problematic cases we are looking for the timestamps also
are separated by a quite short time.)
Yes, this means there is a race condition; what if the document is
modified behind out back right after we have persisted it, before we
ask for its timestamp? It would be much better if the persisting
operation atomically also told what the timestamp of the document in
the storage is after persisting, but alas, WOPI doesn't do that.
Rename the DocumentBroker::origDocumentLastModifiedTime field to
_documentLastModifiedTime as that is less misleading. It is not the
"original" document timestamp but the timestamp of the document in its
storage system.
This needs much more work: Ideally the timestamp of the document in
its storage system should be retrieved and checked against the
expected value also before we are about to save it.
But unfortunately experience has shown that the WOPI CheckFileInfo
operation can be expensive, so we'll see what can be done. Ideally
WOPI should contain the optional functionality to return an error if,
when saving a document, its timestamp (and size?) in storage are not
what the saving client expects.
Also add a few FIXME comments.
Change-Id: I5a9b55d4b55a8db0c9ee8638edd368dc0aa325d5
By that time the loleaflet code has already done the greying-out (this
happens at the latest after 10 minutes of inactivity) and marked them
as 'inactive'. Bluntly closing the session seems to work out fine, no
new horribly alarming message is displayed by the loleaflet code in
addition to the already visible 'Inactive document ...' one.
Change-Id: I420f0d7530194e6c8d6f34d7985ab810cde5a76a
Use a new protocol message, 'resetidle' to inform Admin clients
whenever a user has done anything in a document view. This is a
message that Admin clients need to subscribe to.
Also add the current idle time for each document to the 'documents'
message.
To reduce protocol chatter, the idle time is updated at most once per
10 s.
Change-Id: I418e82b05048a3628f21dcd240ccd974b3a01356
Reviewed-on: https://gerrit.libreoffice.org/31653
Reviewed-by: Tor Lillqvist <tml@collabora.com>
Tested-by: Tor Lillqvist <tml@collabora.com>
