Testing with libdbus 1.6.0 on Debian Testing failed because the
lib changed some behavior: instead of looking up the owner of
a certain bus name immediately, it now does that when invoking
a method. Therefore the check for "have connection" in NetworkManager
and ConnMan client code was too simplistic and missed the fact that
both were not usable, causing the server to assume that HTTP was down
while in reality it should have assumed it to be up.
If the command line tool returned before test-dbus.py recorded
all D-Bus events of that session, then the check of the events
failed. Need to wait until "status: done" is also seen via D-Bus.
Not getting it will result in test timeouts.
Let the bundle .deb depend on libical if the lib was enabled during
compilation (for example, for CalDAV). This ensures that it gets
installed on systems which otherwise don't have it.
When libical and libecal were not installed, trying to use the CalDAV
backend for VEVENTs segfaulted because it depends on libical and did
not check properly for it. Only affected syncevolution.org binaries.
The root cause is that libical functions were only looked up in
combination with libecal, when compiled with
--enable-evolution-compatibility. Now they are first checked via
libecal (for old libecal which embedded libical) and separately in
libical itself as fallback.
Instead of using delays to kill processes at the right time, watch old
and new debug output via D-Bus and then kill the processes. To avoid
race conditions, these processes get delayed at the right point.
Added tests for local sync and command line, covering killing of
all involved processes.
The session output needs to go through the server's
message handling, to get the same kind of treatment.
Sending it to the server's parent logger was missing
previously and thus it did not get sent to the
servers stdout. Found while looking at test-dbus.py
logs, which were incomplete.
When the D-Bus helper quit unexpectedly, the session
was closed immediately. That is necessary as a workaround
for D-Bus libdbus, which does not properly shut down
pending method calls when the connection goes down.
However, it prevents processing further events that might
still be pending. So as a workaround for the workaround
let's run the session shutdown with an internal failure
in an idle callback, which will happen after processing all
pending events.
The API supported repeating timeouts, but none of the users of
it needed that. To fit the API, they were forced to return a boolean
value, which prevented binding to callbacks without such a
return value.
That'll be useful, so let's simplify the code...
The helper process only detected that the parent failed when
it tried to log something while the parent had already shut down
the D-Bus connection. Even that did not work reliably and differed
between D-Bus libdbus and GIO.
Now logging ignores failures to send the message (done the same way
as in the syncevo-dbus-helper -> syncevo-dbus-server communication).
Parent failures are detected via the ForkExecChild::m_onQuit signal.
They set the "abort" state permanently until the helper process
terminates.
Found while testing some new failure test cases in combination with
D-Bus libdbus.
The EDS backend in the syncevolution.org packages are compiled
for the API in EDS <= 3.4. With some tricks (dynamically
loading libs) it works across a range of libecal and libebook
releases.
Now the syncevolution-evolution meta package declares that
it depends on one of these libecal/ebook libraries (was missing
earlier), to ensure that the libs get installed on a system
which did not have them already.
The dynamic loading will no longer attempt to work with
EDS 3.6. For EDS 3.6 it will be necessary to update the EClient
variant of the EDS backend and compile the syncevolution.org
binaries differently.
Avoid the abstract base class and implement the GDBusXX::Watch
class directly. This became necessary because moving the GIO
implementation into the .cpp resulted in strange crashes around
the virtual setCallback() method - but only when optimization
was enabled. Compiler bug?!
Simplifying the class hierarchy is the right step either way. It was
done with separate classes initially to avoid dependencies on
gdbus-cxx-bridge.h in code which just needs to interact with the base
functionality, but it turned out that no code needs that.
The previous implementation did not work at all. It relied
on "NameLost" signals with the watched D-Bus name as sender,
but that is not how the signal works. It is sent by the D-Bus
daemon to the old owner of that name if it looses the name.
Instead we need to watch the "NameOwnerChanged" signal and
detect when the peer we are watching is the one who disconnected
from the bus.
Also moved the actual code into the .cpp file because there is
no need to inline all of it.
This problem was found after adding more failure tests to
test-dbus.py. The real-world effect was that syncevo-dbus-server did
not clean up properly when clients disconnected early and that the
command line kept running after syncevo-dbus-server crashed. Did not
affect syncevolution.org binaries, which do not use D-Bus GIO.
The static template method doesn't have to be in the header
file (doesn't depend on template parameters). Moving it into
the .cpp file to minimize compile and executable size.
Runtime work can be minimized by holding the instance
temporarily in a simpler auto pointer before moving it
into the final shared pointer.
Another reason was that compiling with optimization enabled
produced binaries which crashed in that code. However, moving
it did not help.
An asynchronous D-Bus call will invoke the callback even if
the call instance itself was already deleted. Therefore binding
the this pointer of the call instance owner is not safe. This
caused use-after-free errors in local sync (found during testing).
Add a weak pointer to itself to each LocalTransportAgent
instance and use that weak pointers so that the callback
does not crash when it happens to late.
An asynchronous D-Bus call will invoke the callback even if
the call instance itself was already deleted. Therefore binding
the this pointer of the call instance owner is not safe. This
caused use-after-free errors in local sync (found during testing).
Use weak pointers as a precaution where it can be done easily.
Some remaining start() calls still need to be converted.
On several occasions, boost::bind() is used with a this
pointer as second parameter. This is only safe as long as
the functor is only called when the instance is still valid.
After including BoostHelper.h it is also possible to use
a weak pointer. The result is a functor which does not
prevent deleting the instance and doesn't do anything
when the instance is gone when called.
Nightly builds with --enable-doc tended to fail occassionally due to
download errors of the current docbook XSL from SourceForge. Using
a local copy of those files avoids that problem, speeds up compilation
and gives us some control over potentially incompatible changes
in the upstream docbook XSL.
Google Calendar sometimes returns redirect requests to human-readable
web sites (an "unavailable" page, a login form). This is of course
bogus when the client is an automated CalDAV client.
The "unavailable.html" case was already handled. Made it a bit more
flexible to also catch possible variations of it (additional
parameters, https instead of http).
Added the https://accounts.google.com/ServiceLogin case. Not sure
whether retrying will help in that case, but there's not much else
that SyncEvolution can do.
The new testInsertTwice checks what the backend does when the same
item already exists in the database. This is only done for test data
which has a UID (i.e. for calendar data, but not for contacts). This
test exercises the code which previously failed with 412 "Precondition
Failed" in the CalDAVMemo backend.
When asked to insert a VJOURNAL which already existed (= same UID),
CalDAV servers respond with a 412 "Precondition failed" error. This
needs to be detected and translated into an "item needs to be merged"
result so that the engine can load the existing item, merge the data,
and then write back.
A test for this, testInsertTwice, will be committed separately. The
code was written so that it handles the same error when using CardDAV.
However, this was not tested because CardDAV test data does not have a
UID (wouldn't trigger the problem) and Radicale did not report 412 when
adding the UID.
WebDAVSource, in contrast to CalDAVSource, does not check for UID
conflicts before the PUT. As a result, testSimpleInsert and
testLocalDeleteAll were failing when the VJOURNAL test item had
already been imported.
Need to strip the UID in cases where it is not guaranteed that the
item is not already in the database. Cases where that can be taken
for granted because of a preceeding delete continue to use the UID
embedded in the test data (but don't rely on it, because update()
without LUID will find the existing item and use its LUID).
Instead of relying on catching an exception, better use the new
"expected status codes" feature and check for 412 as part of the
normal switch statement.
Some WebDAV requests might fail with a non-okay status code that the
caller expects. This was reported via an exception. But
SyncEvolution's design uses exceptions only for non-normal
incidencences. Therefore better allow the caller to indicate which
status codes are expected and return normally from run() and
checkError() when those are encountered, without retrying and without
throwing an exception.
The testConversion test was conceptually broken: it only passed for
peers where synccompare filtered out X-EVOLUTION-UI-SLOT, because that
parameter always gets dropped during import/export. It's disable by
default and needs to be enabled via DevInf, which is never received
during this test.
synccompare also might have hidden other kinds of data loss, depending
on which workarounds were enabled via CLIENT_TEST_SERVER.
The fix is to unset CLIENT_TEST_SERVER and enable removal of
X-EVOLUTION-UI-SLOT via CLIENT_TEST_STRIP_PARAMETERS.
The testExtensions test was broken since creating sources reads
CLIENT_TEST_SERVER to find the databases, because CLIENT_TEST_SERVER
was temporarily cleared in testExtensions.
Funambol's OneMedia sends UID, but not RECURRENCE-ID. That becomes a
problem when multiple events of the same event series are added to a
backend which follows the iCalendar 2.0 standard (CalDAV, EDS, KDE),
because these events all look like the master event, and there can be
only one of those.
SyncEvolution now strips the UID from all events coming from any
Funambol server (regardless of the version). If a future Funambol
server release adds support for both UID and RECURRENCE-ID, then
SyncEvolution will have to be updated to take advantage of the
improved server. Because the RECURRENCE-ID is also getting
stripped (despite not being set at the moment), SyncEvolution should
continue to work as it does now even if the server changes.
It would have been nice to limit this workaround to affected Funambol
server versions, but an inquiry on the Funambol mailing list didn't
get a reply, therefore SyncEvolution is playing it safe and assumes
that all future Funambol releases will have the same problem.
In some cases data with a very long UID wasn't handled correctly,
causing the out-going data to be malformed and probably causing a
rejection by the server.
The root-cause was two-fold:
- extractUID() didn't expect folding. Normally it deals only with
data encoded by libsynthesis, which does not use folding; the unexpected
exception was data which gets imported directly (--import).
- setResourceName() used std::string::replace() incorrectly: second
parameter is length, not end offset, of the data to be replaced.
When writing calendar items into a backend storage as iCalendar 2.0 or
vCalendar 1.0, they should have DTSTAMP and LAST-MODIFIED values. DTSTAMP
is expected by some CalDAV servers (like Apple). LAST-MODIFIED is usually
added by the storage, but not always.
In the text/plain -> syncevolution -> text/calendar -> Radicale -> EDS
-> syncevolution chain the LAST-MODIFIED was not added by Radicale, which caused
problems for change tracking in an EDS-based SyncEvolution.
The command line --status operation did not complete when applied to a
CalDAV/CardDAV source. Instead it aborted because the operation took a
code path where the backend was not fully initialized.
Forking helper processes has better support for debugging. It disables
this output redirection and inherited loggers before executing the
helper. Not a big change because the parent probably already disabled
output redirection.
Added a "target side of local sync ready" INFO message to introduce
the output which has the target context in the [INFO] tag. The sync report
from the target side now has the target context embedded in brackets
after the "Changes applied during synchronization" header, to avoid
ambiguities.
Output from the target side of a local sync was passed through stderr
redirection as chunks of text to the frontends. This had several
drawbacks:
- forwarding only happened when the local sync parent was processing
the output redirection, which (due to limitations of the implementation)
only happens when it needs to print something itself
- debug messages were not forwarded
- message boundaries might have been lost
In particular INFO messages about delays on the target side are
relevant while the sync runs and need to be shown immediately.
Now the output is passed through D-Bus, which happens immediately,
preserves message boundaries and is done for all output. The frontend
can decide separately whether it shows debug messages (not currently
supported by the command line tool).
Implementing this required extending the D-Bus API. The
Server.LogOutput signal now has an additional "process name"
parameter. Normally it is empty. For messages originating from the
target side, it carries that extra target context string.
This D-Bus API change is backward compatible. Older clients can still
subscribe to and decode the LogOutput messages, they'll simply ignore
the extra parameter. Newer clients expecting that extra parameter
won't work with an older D-Bus daemon: they'll fail to decode the
D-Bus message.
This revealed that the last error messages in a session was
incorrectly attributed to the syncevo-dbus-server. Might also have
happened with several other error messages. Now everything happening
in the server while working on code related to a session is logged as
coming from that sessions. It's not perfect either (some of the output
could be from unrelated events encountered indirectly while running
that code), but it should be better than before.
The patch changes the handling or errors in the local sync parent
slightly: because it logs real ERROR messages now instead of plain
text, it'll record the child's errors in its own sync report. That's
okay, user's typically shouldn't have to care about where the error
occurred. The D-Bus tests need to be adapted for this, because it
removes the "failure in local sync child" from the sync report.
Another, more internal change is that the syncevo-local-sync helper
does its own output redirection instead of relying on the stderr
handling of the parent. That way libneon debug output ends up in the
log file of the child side (where it belongs) and not in the parent's
log.
Sometimes the backend has to resend requests because of temporary
issues. If the problem turned out to be permanent, there was a long
period of time, retryDuration=5 minutes to be precice, in which no
visible progress happened.
Now SyncEvolution's WebDAV backend will print a message like this
before going to sleep until it is time to retry:
[INFO @client-test-googlecalendar] operation temporarily (?) failed, going to retry in 5.0s before giving up in 18.4s: PROPFIND: Neon error code 1: 401 Unauthorized
The uncertainty comes from several factors. In this example, the 401
might indicate a permanent problem (wrong credentials), or it could be
Google reporting a temporary authorization problem which is (probably)
meant to slow down the client while it asks the user to re-enter the
password. SyncEvolution only asks for passwords once, so it tries
again with the same password if it was successful with it in the
past. Otherwise it gives up immediately.
Another dubious example are name server lookup errors. They can be
ermanent (wrong host name) or temporary (name server
down). SyncEvolution errs on the side of retrying, to avoid
interrupting an operation which still has a chance to continue.
Currently this output is only visible in combination with --daemon=no,
because when using syncevo-dbus-server the output is not passed on
properly.
--print-databases for an existing configuration did not look up
passwords stored in a keyring, causing the operation to fail for
backends like CalDAV/CardDAV where credentials are required.
Overriding source properties in that case also only worked when using
the unqualified property name ("databasePassword=foo") but not when
using the source name as prefix ("calendar/databasePassword=foo").
The code which marked a failure as "ignored" unintentionally
cleared the overall error flag, causing client-test runs
with ignored and real failures to return 0 = success.
