And check for save-failed first before finding
the session, as otherwise the session is useless.
Change-Id: I3eb45e54872547eb36765b2c8409e1aa51aee589
Reviewed-on: https://gerrit.libreoffice.org/32611
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Avoid unnecessary code under lock
or when DocBrokers is updated,
which requires removal on exception.
Change-Id: Id01aed42cd66616b910b7e16a8a1ed6c1d1e74b9
Reviewed-on: https://gerrit.libreoffice.org/32558
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
... and print error in the logs. So that if there is some problem
in the setup and we are not able to interact with WOPI host for some
reason, we print it in the error logs clearly.
Convert to new style logging - LOG_XXX macros.
Change-Id: I53ce4f61136ffd256b0eae8712dc7f22a620e2bf
Simplifies the code and makes it less error prone.
Also add more logging on modifying DocBrokers.
Change-Id: I861495912eb4994a32b1ccaccc181fb1aad26e1f
Reviewed-on: https://gerrit.libreoffice.org/32295
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>
Since the local path already ends in foreslash,
no need to keep the on in provided by the client.
Change-Id: Ia2bc24c7faa84509f9ec18deefb14cad2858e856
Reviewed-on: https://gerrit.libreoffice.org/32288
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
The information in StorageBase::_fileInfo is not expensive to
construct. What might be expensive we were doing anyway for each time
a session connects to the document. So just update _fileInfo whenever
we fetch currrent information about the document in its storage, which
is useful, as the timestamp and size after all will change whenever
the document is persisted.
Change-Id: I173394c88b4d6448ad5bf1ab9b41694cffdf1ff4
And not as the current time. The getLocalFileInfo() and
getWOPIFileInfo() functions will reset it anyway.
Change-Id: Ief821482cf789113cecd85e5d59afecf6ad2c3ab
It would be another thing if it would be abstract and overridden in
the concrete derived classes. But now it just returns the _fileInfo
field that is set by the separate getLocalFileInfo() and
getWOPIFileInfo() functions. Possibly this should be re-designed
and/or stuff re-named for clarity.
Change-Id: I30d01277ac0c6bd4b9daa317aade319b6ef39342
Poco::Timestamp::fromEpochTime() is a static member function that
returns a Timestamp.
Let's hope this doesn't introduce any regression.
Change-Id: I0997c4c3128ec07d5db76dbf3ecc388cd19ac2ac
Initial effort was setting it when we are compiling with
--enable-debug or when not, unit tests are running, but since we don't
run the unit tests in absence of --enable-debug anyways,
there is no use for this #else code block.
Change-Id: I9d7c66a18be160a47afd2bf336d89d50d4f2463e
