These Log::foo() calls are in ClientRequestHandler::handleClientRequest(),
not in ClientRequestHandler::handleRequest().
Actually I wonder why we show the name of the function in this handful of
places. We don't do it in general. Consistency, bah.
I fear they get can out of sync when HTTPCrashTest::killLoKitProcesses()
kills a loolkit process, and this then causes HTTPWSError::testMaxDocuments()
to fail. This is to help debugging that.
Haven't fully understood what is going on yet. But one thing is sure:
It is a bad idea to duplicate the same state information in two
places, we shouldn't really use that separate NumDocBrokers
variable.
Probably also NumConnections tracks state that can easily be
calculated from the data structures, but maybe NumConnections does not
that easily get out of sync.
The waiting for the expected number of loolkit processes after that
test never succeeded. That apparently was caused by the fact that the
loolwsd process is waiting for up to COMMAND_TIMEOUT_MS (five seconds
currently) for an auto-save of the document to succeed, and that never
happens in this case.
countLoolKitProcesses() on the other hand was waiting only a bit over
three seconds (calculated in a complicated way from POLL_TIMEOUT_MS
and a few magic numbers) for the number of loolkit processes to reach
the expected number. Or something like that.
COMMAND_TIMEOUT_MS=5000 and POLL_TIMEOUT_MS=COMMAND_TIMEOUT_MS/10 =>
500. We used to have in countLoolKitProcesses():
sleepMs=POLL_TIMEOUT_MS/3 => 166 and repeat=(3000/sleepMs)+1 =>
19. 19*166 => 3154.
Fix by calculating the max time to wait for the expected number of
loolkit processes in countLoolKitProcesses() in a different way, so
that it is always longer than COMMAND_TIMEOUT_MS.
Use the previously unused UnauthorizedRequestException for this, and
throw a such in StorageBase::create() when the WOPI host doesn't match
any of those configured.
In a developer debug build, without access to any real WOPI
functionality, you can test by setting the FAKE_UNAUTHORIZED
environment variable and attempting to edit a plain local file:
URI. That will cause such an exception to be thrown in that function.
Catch that UnauthorizedRequestException in
ClientRequestHandler::handleGetRequest(), and send an 'error:
cmd=internal kind=unauthorized' message to the client. Handle that in
loleaflet in the same place where the 'error: cmd=internal
kild=diskfull' message is handled, and in the same fashion, giving up
on the document.
Actually, using exceptions for relatively non-exceptional situations
like this is lame and makes understanding the code harder, but that is
just my personal preference...
FIXME: By the time StorageBase::create() gets called we have already
sent three 'statusindicator:' messages ('find', 'connect', and
'ready') to the client. We should ideally do the checks we do in
StorageBase::create() much earlier.
Also consider that ClientRequestHandler::handleClientRequest() has
code that catches UnauthorizedRequestException and
BadRequestException, and tries to set the HTTP response in those
cases. I am not sure if that functionality has ever been exercised,
though. Currently, we upgrade the HTTP connection to WebSocket early,
and only after that we check whether the WOPI host is authorized
etc. By that time it is too late to return an HTTP response to the
user. If that even is what we ideally should do? If not, then we
probably should drop the code that constructs HTTP responses and
attempts to send them.
Also, if I, as a test, force an HTTPResponse::HTTP_BAD_REQUEST to be
sent before the HTTP connection is upgraded to WebSocket, loleaflet
throws up the generic "Well, this is embarrassing" dialog anyway. At
least in Firefox on Linux. (Instead of the browser showing some own
dialog, which I was half-expecting to happen.)
Doing a plain CPPUNIT_TEST_NAME='somettest' make check will
result in executing all the tests in external test suite. This is
a problem when one wants to execute only internal tests (unit-*
ones) as test harness first executes those followed by all of the
tests in external test-suites.
Lets execute all the tests only when no CPPUNIT_TEST_NAME is
provided, and ignore when it is provided but no match is found.
Change-Id: I7e40b6f3124e6965a86cfb6395d246df3b5c17ba
There doesn't seem to be any failure on this test anymore.
Consecutive 20 runs of this test gives no failure, so lets enable
it again.
Change-Id: I77ddd1c36d18162bdc75fd24d51c1a2df22f749d
addSession() now also loads the document, so querying docbroker
for load duration before it doesn't make any sense.
Change-Id: I3c60bef5e2054878ba695b8f76b6800cdedffe8d
Pass std::string instead of char* and length where
a string is always constructed anyway. Also cleaner
and safer code.
Change-Id: I1c9341e2c81bbdb7adeb29d3fba59849b2617e95
Reviewed-on: https://gerrit.libreoffice.org/29954
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This avoids early connection failures causing the
session to linger in the Broker indefinetely.
Change-Id: Ibe2b5b386ed1cd6e12e68735bff60b15957188cf
Reviewed-on: https://gerrit.libreoffice.org/29952
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
When upon cleanup we segfault we want to avoid
forcible exit using _Exit(). This was done by
an unconditional wait (sleep).
This patch changes that mechanism into a latch
(mutex) that traps the exit when the sig handler
is invoked, therby preventing exit as long as
necessary for the sighandler to finish.
Change-Id: Ifc8e972be12645a1b310d4cb4e3a4172afc31327
Reviewed-on: https://gerrit.libreoffice.org/29945
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
No need to do a partial cleanup in the kit
when forkit recycles the disk. This avoid
both programatic errors (as the comments in
the removed code explains) as well as hammering
the disk from multiple processes.
By leaving all disk cleanup to forkit we
guarantee safety and that only one process
does disk cleanup, and sequentially at that.
N.B. Kit processes are still responsible for
setting up the jail and the LO binaries etc.
So disk IO is not strictly sequential from a
single process.
Change-Id: Ia6768ab87df71a83a6676c3d52da3d6797f717fc
Reviewed-on: https://gerrit.libreoffice.org/29944
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Reduces the number of WOPI calls made during a document load. Earlier
effort was made in edfd3266f8
This commit cleans up and uses better approach for the same.
Other than that, access token of each session is now correctly
used when interacting with the storage. Earlier, we used to
use the same access token for each upload to storage which means
that irrespective of who clicked the save button, changes to the
document were only made on behalf of one person (of whom the
access token is used). This is fixed now.
Also includes minor cleanup left and right.
Change-Id: Id32702ff02aea4f63b7cc6afa9f62664807bb57d
Reviewed-on: https://gerrit.libreoffice.org/29931
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This helps debugging and tracing issues as this single log
entry links the three most important pieces together:
the user, how Core references the user's view to a
given document, and how WSD references the same.
Change-Id: I7bf88504b43eed85d40e6f8bc9c3bad713f372da
Reviewed-on: https://gerrit.libreoffice.org/29935
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This reduces the number of fileinfo calls made to storage. These calls can
be expensive in storage such as WOPI where loolwsd needs to
interact with another server to get the file information. Use the
same storage object once created so that fileinfo can be
cached and returned quickly for subsequent such calls.
3 GetFileInfo WOPI calls are now merged into 1.
Change-Id: I56c3d23d3d6d7dc3a4b42433f51304dac28a12e8
This saves us from encoding/decoding mess of URIs. Poco::URI is
flexible enough and can give encoded or decoded version whenever
required, so lets avoid storing the URI in std::string where we
have no information about whether its encoded or not.
Change-Id: I4a6979e13b20d9f56fc5a0baa630cb1b35ca33b0
