Passwords are cached after the initial check as temporary property
values. The explicit string members are obsolete and can be removed
together with the code using them.
Like everything else that waits for a certain event on the main loop,
SYNCEVO_GLIB_CALL_SYNC() should also better use GRunWhile(). This is
necessary to be usable in threads.
When clients like the GTK sync-ui stored a password, it was always
stored as plain text in the config.ini file by the
syncevo-dbus-server. The necessary code for redirecting the password
storage in a keyring (GNOME or KWallet) simply wasn't called in that
case.
The command line tool, even when using the D-Bus server to run the
operation, had the necessary code active and thus was not affected.
uint16 happened to work when compiling with a recent GNOME stack, but
without that uint16 is not defined. The right approach is to use
stdint.h and uint16_t.
Both maintaining the map items inside the Synthesis engine and storing
them in .ini hash config nodes inside SyncEvolution are fairly heavy
operations which are not needed at all during an ephemeral sync (= no
meta data stored, done by the PIM Manager when triggering a pure PBAP
sync).
Using the new Synthesis CA_ResumeSupported DB capability it is
possible to suppress these operations without having to fiddle with
the Synthesis DB API that SyncEvolution provides. To make it possible
at the DB layer to detect that the meta data is not needed, the
ConfigNode passed to it must be marked as volatile.
This change sped up a sync with 10000 unmodified, known items from 38s
to 23s.
The synthesisID value is required for each Synthesis source progress
event, which can be fairly frequent (more than one per item). Instead
of going down to the underlying .ini config node each time, cache the
value in the SyncSourceConfig layer.
Syncing was slowed down by fowarding all log messages from the local
sync helper to its parent and from the D-Bus helper to
syncevo-dbus-server. Quite often, the log messages then were simply
discarded by the recipient. To speed up syncing, better filter at the
source.
The syncevo-dbus-helper is told which messages are relevant by
forwarding the syncevo-dbus-server "D-Bus log level" command line
setting to the helper process as part of its argv parameters.
The synevo-local-sync helper applies its own log level now also to the
messages sent to the parent. This ensures that messages stored in the
client log also show up in the parent log (unless the parent has more
restrictive settints, which is uncommon) and that INFO/SHOW messages
still reach the user.
For each incoming change, one INFO line with "received x[/out of y]"
was printed, immediately followed by another line with total counts
"added x, updated y, removed z". For each outgoing change, a "sent
x[/out of y]" was printed.
In addition, these changes were forwarded to the D-Bus server where a
"percent complete" was calculated and broadcasted to clients. All of
that caused a very high overhead for every single change, even if the
actual logging was off. The syncevo-dbus-server was constantly
consuming CPU time during a sync when it should have been mostly idle.
To avoid this overhead, the updated received/sent numbers that come
from the Synthesis engine are now cached and only processed when done
with a SyncML message or some other event happens (whatever happens
first).
To keep the implementation simple, the "added x, updated y, removed z"
information is ignored completely and no longer appears in the output.
As a result, syncevo-dbus-server is now almost completely idle during
a running sync with no log output. Such a sync involving 10000 contacts
was sped up from 37s to 26s total runtime.
Set SYNCEVOLUTION_DBUS_HELPER_VGDB=1, add --vgdb-error=1 --vgdb=yes
to VALGRIND_ARGS, run test, wait for vgdb message in valgrind*.out files,
attach as instructed.
With --vgdb-error=0, all processes block during startup, waiting for
the debugger to attach.
The previous attempt with concurrent reading while listing IDs did not
work, that listing must complete before the SyncML client contacts the
server. What works is transfering and parsing after the engine starts
to ask for the actual data.
For that we need to list IDs in advance. We use GetSize() for that.
If contacts get deleted while we read, getting the data for the
contacts at the end of the range will fail with 404, which is
understood by the Synthesis engine and leads to ignoring the ID, as
intended.
If contacts get added while we read, we will ignore them even if they
happen to be in the result of PullAll. The next sync will include
them.
When doing a PBAP sync, PIM manager asks the D-Bus sync helper to set
its SYNCEVOLUTION_PBAP_SYNC to "incremental". If the env variable
is already set, it does not get overwritten, which allows overriding
this default.
Depending on the SYNCEVOLUTION_PBAP_SYNC env variable, syncing reads
all properties as configured ("all"), excludes photos ("text") or
first text, then all ("incremental").
When excluding photos, only known properties get requested. This
avoids issues with phones which reject the request when enabling
properties via the bit flags. This also helps with
"databaseFormat=^PHOTO".
When excluding photos, the vcard merge script as used by EDS ensures
that existing photo data is preserved. This only works during a slow
sync (merge script not called otherwise, okay for PBAP because it
always syncs in slow sync) and EDS (other backends do not use the
merge script, okay at the moment because PIM Manager is hard-coded to
use EDS).
The PBAP backend must be aware of the PBAP sync mode and request a
second cycle, which again must be a slow sync. This only works because
the sync engine is aware of the special mode and sets a new session
variable "keepPhotoData". It would be better to have the PBAP backend
send CTCap with PHOTO marked as not supported for text-only syncs and
enabled when sending PHOTO data, but that is considerably harder to
implement (CTCap cannot be adjusted at runtime).
beginSync() may only ask for a slow sync when not already called
for one. That's what the command line tool does when accessing
items. It fails when getting the 508 status.
The original goal of overlapping syncing with download has not been
achieved yet. It turned out that all item IDs get requested before
syncing starts, which thus depends on downloading all items in the current
implementation. Can be fixed by making up IDs based on the number of
existing items (see GetSize() in PBAP) and then downloading later when
the data is needed.
If PHOTO and/or GEO were the only modified properties during a slow
sync, the updated item was not written into local storage because
they were marked as compare="never" = "not relevant".
For PHOTO this was intentional in the sample config, with the
rationale that local storages often don't store the data exactly as
requested. When that happens, comparing the data would lead to
unnecessary writes. But EDS and probably all other local SyncEvolution
storages (KDE, file) store the photo exactly as requested, so not
considering changes had the undesirable effect of not always writing
new photo data.
For GEO, ignoring it was accidental.
A special merge script handles EDS file:// photo URIs. When the
loosing item has the data in a file and the winning item has binary
data, the data in the file may still be up-to-date, so before allowing
MERGEFIELDS() to overwrite the file reference with binary data and
thus forcing EDS to write a new file, check the content. If it
matches, use the file reference in both items.
Performance is improved by requesting multiple contacts at once and
overlapping reading with processing. On a fast system (SSD, CPU fast
enough to not be the limiting factor), testpim.py's testSync takes 8
seconds for a "match" sync where 1000 contacts get loaded and compared
against the same set of contacts. Read-ahead with only 1 contact per
query speeds that up to 6.7s due to overlapping IO and
processing. Read-ahead with the default 50 contacts per query takes
5.5s. It does not get much faster with larger queries.
While returning items from one cache populated with a single
e_book_client_get_contacts() call, another query is started to overlap
processing and loading.
To achieve efficient read-ahead, the backend relies on the hint given
to it via setReadAheadOrder(). As soon as it detects that a contact is
not the next one according to that order, it switches back to reading
one contact at a time. This happens during the write phase of a sync
where the Synthesis engine needs to read, update, and write back
changes based on updates sent by the peer.
Cache statistics show that this works well for --print-items, --export
and slow syncs.
Writing into the database must invalidate the corresponding cached
contact. Otherwise the backup operation after a sync may end up
reading stale data.
Trying to predict in the SyncSource which items will be needed is
hard. It depends what the item is retrieved for (sync or
backup/printing) and on the sync mode (during a sync).
Instead of guessing, better have the user of the source tell the
source in advance what it will read. In most cases this is cheap
because it does not involve copying of items luids ("read all items",
"read new or modified items"). The source then can use its own internal
mechanisms to figure out what that means.
Only extracting specific items specified on the command line and the
backup operation must explicitly pass an ordered list of luids. For
the sake of simplicity, do that in a std::vector even though it
involves copying.
The
while (<something>) g_main_context_iterate(NULL, true);
pattern only works in the main thread. As soon as multiple
threads are allowed to process events, race conditions occur,
as described before.
But the pattern is useful, so support it in all threads by
shifting the check into the main thread, which will then notify
other threads via a condition variable when something changes.
gcc complained about the "protected" m_mutex access. Apparently
it did not realize that the access was to a base class. An explicit
get() solves that. Another way to get the pointer is a type cast.
Derive from SyncSource and SyncSourceSession directly instead of going
through TrackingSyncSource. This allows removing several dummy methods
that we have to implement for TrackingSyncSource and allows
reporting existing items incrementally, instead of having to load all
of them at once for the old listAllItems().
Contacts are now already reported to the engine while their transfer
still runs. That depends on monitoring the temporary file, remapping
the larger file and continuing parsing where the previous parsing
stopped.
This only works with obexd 0.47 and obexd from Bluez 5, because it
depends on the temporary file interface. The older PullAll did not
return any data until it had downloaded everything.
Because it isn't known when the contact data will be needed, the backend
still maintains the mapping from ID to vCard data for all contacts seen
in the current session. Because that memory is backed by a temporary file
system, unused memory can be swapped out (and in) well by the OS.
If the file is in a ram-based temp file system, then it may also not
matter at all that the file gets mapped multiple times.
Instead of reading all item IDs, then iterating over them, process
each new ID as soon as it is available. With sources that support
incremental reading (only the PBAP source at the moment) that provides
output sooner and is a bit more memory efficient.
remove() is useful when want to continue using the file but also want
to ensure that it gets deleted when we crash.
moreData() can be used to determine if the file has grown since the
last time that map() was called. In other words, it supports
processing data while obexd still writes into the file.
The previous commit "PBAP: fix support for obexd >= 0.47 and < Bluez 5"
made the backend work with obexd 0.48 and broke it with 0.47. That's because
there was another API change between 0.47 and 0.48, which wasn't known
at the time of that commit.
SyncEvolution now works with 0.47 and does not work with 0.48. This choice
was made because 0.47 supports the file-based data transfer (same as in Bluez 5
and thus useful for testing when Bluez 5 is not available) and 0.47 still
compiles against older Bluez versions (which makes it easier to use than 0.48).
More recent GNOME Python bindings are provided by gobject
introspection. The traditional gobject/glib modules no
longer exist.
The API is similar enough that we just need to adapt importing: if
importing the normal modules fails, try importing from gi.repository
instead.
EDS 3.8 sets X-EVOLUTION-FILE-AS to "last, first" when a contact is
created without it. This leads again to unnecessary DB updates because
the incoming item that the engine works with doesn't have that field
set.
To mitigate that issue, set FILE_AS (renamed to make the field name
valid in a script) like EDS would do during writing.
The downside is that all incoming items now have FILE_AS set, which
will overwrite a locally stored copy of that property even if the peer
did not store X-EVOLUTION-FILE-AS. Previously, as tested by
testExtension, the local value was preserved. There is no good solution
that works for both use cases, so allow X-EVOLUTION-FILE-AS to get lost
and relax the test.
Traditionally, contacts were modified shortly before writing into EDS
to match with Evolution expectations (must have N, only one CELL TEL,
VOICE flag must be set). During a slow sync, the engine compare the
modified contacts with the unmodified, incoming one. This led to
mismatches and/or merge operations which end up not changing anything
in the DB because the only difference would be removed again before
writing.
To avoid this, define datatypes which include the EDS modifications in
its <incomingscript>. Then the engine works with an item as it would
be written to EDS and correctly determines that the incoming and DB
items are identical.
Found in testpim.py's testSync when applied to the test cases
generated by the Openismus test case generator.
If the sources are used in a mode which doesn't need revision strings,
then skip the retrieval of items from the EDS daemons when we would
normally do it to get the revision string.
Recording the revision of items during a caching sync is unnecessary
because the next sync will not use the information. For item
modifications, the information was not even recorded.
Now a "don't need changes" mode can be set in SyncSource, which is
done for caching and command line item operations. This is better than
second-guessing the mode in which a source is used.
SyncSourceRevision checks that flag and skips updating the
luid->revision mapping if set.
Individual backends can also check the flag and skip calculating the
revision to begin with.
Only works when compiled for EDS >= 3.6. Uses the new ITEM_AGAIN in
SyncSourceSerialize. Combines multiple add/update operations into one
asynchronous batch operation which can overlap with network IO if
the engine supports it.
To take full advantage of batch operations, we must enable out-of-order
execution of commands. Otherwise the engine will never issue more than
one change to us. Do it for SyncEvolution as peer.
TODO: do it for all peers or disable batched processing when it is not
enabled.
SyncSourceSerialize uses ITEM_AGAIN plus a callback in
InsertItemResult to indicate that and how an insert (aka add or
update) operation must be continued.
TrackingSyncSource merely passes that result through until it finally
completes.
Direct, raw access to items as done by the command line and restore
operations is not done asynchronously. The backend does not need to
know how it is used, the SyncSourceSerialize wrapper around the
backend will flush and wait.
Asynchronous deleting is not supported. It would require throwing an
exception or an API change (better - exceptions are for errors!)
because removeItem() has no return code which could be extended.
The wrapper around the actual operation checks if the operation
returned an error or result code (traditional behavior). If not, it
expects a ContinueOperation instance, remembers it and calls it when
the same operation gets called again for the same item.
For add/insert, "same item" is detected based on the KeyH address,
which must not change. For delete, the item local ID is used.
Pre- and post-signals are called exactly once, before the first call
and after the last call of the item.
ContinueOperation is a simple boost::function pointer for now. The
Synthesis engine itself is not able to force completion of the
operation, it just polls. This can lead to many empty messages with
just an Alert inside, thus triggering the "endless loop" protection,
which aborts the sync.
We overcome this limitation in the SyncEvolution layer above the
Synthesis engine: first, we flush pending operations before starting
network IO. This is a good place to batch together all pending
operations. Second, to overcome the "endless loop" problem, we force
a waiting for completion if the last message already was empty. If
that happened, we are done with items and should start sending our
responses.
Binding a function which returns the traditional TSyError still works
because it gets copied transparently into the boost::variant that the
wrapper expects, so no other code in SyncSource or backends needs to
be adapted. Enabling the use of LOCERR_AGAIN in the utility classes
and backends will follow in the next patches.
Check conditions each time they change as part of view updates. This
would have helped to find the invalid check earlier and catches
temporary violations of the invariants.
The assertEqual() expected 'quiesence' to be seen, but runUntil() was not
asked to run until that was seen. Therefore tests failed randomly depending on
timing: they passed if the quiescence signal was processed in the same event
loop run as the change that was waited for and failed if it arrived later.
Fixed by waiting for 'quiesence' explicitly. It should arrive last.
When "Abraham" falls out of the view, it is possible that we have temporarily
no contacts in the view, because first "Abraham" gets reviewed and then
"Benjamin" gets added. The check "at least one contact" therefore was too
strict.
So far, a "check" callback was passed to runUntil() which checked the state
after each main loop run and at regular time intervals. The downside was that
an incorrect state change inside a single main loop run (for example, removing
a contact and adding one in testMerge) was not detected.
To increase coverage of state changes, the check is now also invoked after
each method call in the ViewAgent. If a check fails, it gets recorded as
error in the view and then will get noticed outside of the event loop by the
check that is passed to runUntil().
These checks needs to be installed before running the command line which
changes the state, not just as part of runTime(), because running the command
line also processes glib events.
This insufficient coverage was found when changes in timing caused testMerge
to fail: the previous check for "always exactly one contact" turned out to be
too strict and merely passed when remove + add were processed together. In
practice, the contact's ID changes because folks ties the merged contact to
a different primary store than the unmerged, original contact.
TESTPIM_TEST_SYNC_TESTCASES can be set to the name of a file
containing contacts in vCard 3.0 format with an empty line as
separator. In this mode, testSync() measures the duration of cleaning
the local cache, importing all test cases, matching them against the
cache (which must not write into the cache!) and removing the cached
contacts.
Typically the peer configs get created from scratch, in particular
when testing with testpim.py. In that case the log level cannot be set
in advance and doing it via the D-Bus API is also not supported.
Therefore, for debugging, use SYNCEVOLUTION_LOGLEVEL=<level> to create
peers with a specific log level.
The process name has become essential to distinguish messages
from local sync parent and child, because the parent will include
the child's output in its own log.
We cannot pass a process name to the Synthesis logging mechanism, so
use the logging prefix instead. Relies on a patch to libsynthesis to
enable the logging of that prefix.
The localized DisplayName strings must be removed explicitly. To find them,
list all properties in the main section and then remove if the prefix
matches.
Automatically calls g_strfreev(). Accessing entries in the array must
be done with at(), overloading the [] operator led to a "operator
ambiguous" warning from gcc on Debian Stable in x86.
CondTimedWaitGLib() would not have returned immediately for
milliSecondsToWait == 0, as expected by the Synthesis engine.
Not sure whether it mattered in practice, though.
doFilter() gets extended to take (<full name>, <vcard>) tuples in
addition to the full name alone. Then this is used to create one large
vCard that is suitable for testing (field content unique, all fields
set, etc.) with various filters. All field tests are covered with at
least one positive and one negative case.
The operation is a runtime parameter of different classes, whereas
extracting the right values to compare via the operation is hard-coded
at compile time. This is a rather arbitrary compromise between code
duplication, simplicity and performance (which, in fact, was not
measured at all).
The code for selecting case-sensitivity and the normalization before
the string operations is shared with the older 'any-contains'
operation.
The new TestContacts.testFilterLogic uses the same infrastructure as
the language tests and covers some combinations of 'and' and 'or'.
In some cases, Python lists had to be avoided in favor of tuples,
because Python cannot automatically map the lists to a 'av' array of
variants, while sending a tuple enumerating its types can be sent and
gets accepted by PIM Manager.
This changes the signature of the filter parameter in Search(),
RefineSearch() and ReplaceSearch() from 'as' (array of strings) to
'av' (array of variants). Allowed entries in the variant are arrays
containing strings and/or other such arrays (recursive!).
A single string as value is not supported, which is the reason why
'av' instead of just plain 'v' makes sense (mismatches can already be
found in the sender, potentially at compile time). It also helps
Python choose the right type when asked to send an empty list. When
just using 'v', Python cannot decide automatically.
Error messages include a backtrace of all terms that the current,
faulty term was included in. That helps to locate the error in a
potentially larger filter.
The scope of supported searches has not changed (yet).
The signature of the new type is just a 'v' for variant. Recursion is
only supported in combination with std::vector. This is done to keep
implementation simpler. std::vector was chosen over other collections
because it is easier to work with in method calls, the main purpose
of the new type.
The conversion from D-Bus messages accepts anything which can be mapped
into the variant: arrays, tuples, plain types, and of course variants
containing those. This helps with backward compatibility (one can
turn an interface which took a fixed type like std::vector<std::string>
into something with a recursive variant) and with Python, because
Python sends arrays and tuples without converting into variants
when the app uses those simpler types.
One caveat exists with sending an empty list in Python: when using 'v'
as signature in the interface, Python doesn't know how to send the
empty list without assistance by the programmer (as in dbus.Array([],
signature='s'). If possible, avoid the plain 'v' in an interface in
favor of something like 'av' (= std::vector<boost::variant>).
There is a difference in EDS between removing the database definition
from the ESourceRegistry (which makes the data unaccessible via EDS)
and removing the actual database. EDS itself only removes the definition
and leaves the data around to be garbage-collected eventually. This is
not what we want for the PIM Manager API; the API makes a stronger
guarantee that data is really gone.
Fixed by introducing a new mode flag for the deleteDatabase() method
and deleting the directory of the source directly in the EDS backend,
if requested by the caller.
The syncevolution command line tool will use the default mode and thus
keep the data around, while the PIM Manager forces the removal of
data.
Instead of hard-coding a specific "Backend Summary Setup" in
SyncEvolution, copy the config of the system database. That way
special flags (like the desired "Backend Summary Setup" for local
address books) can be set on a system-wide basis and without having to
modify or configure SyncEvolution.
Because EDS has no APIs to clone an ESource or turn a .source file
into a new ESource, SyncEvolution has to resort to manipulating and
creating the keyfile directly.
When an asynchronous method call failed with an exception, the code
DBusResult got destructed first before handling the exception and then
sent a generic, uninformative error back to the caller. A test
demonstrating that for the PIM Manager SetPeer() with an invalid UID
be committed separately.
To fix this, the DBusResult must know whether it is really responsible
for replying to the method message. The method call handlers
themselves do not know whether any of their parameters is a
DBusResult, so we have to activate the automatic reply indirectly, via
a guard instance with the same life time as the handler.
When the guard destructs, it can check whether the handler has
replied, because the handler then clears the message pointers. If it
hasn't, the DBusResult becomes the owner of the message. For this to
work, an extra try/catch block is necessary. Otherwise exception
handling would still occur too late, at a time when the guard is gone
and the DBusResult has sent the generic error.
Some other classes had to be adapted to allow for this extra level of
indirection. While doing so, another bug was found.
Retrieving the interface and member used when calling a method
returned the path instead, due to a cut-and-paste error. Fixed. Not
used in practice.
Having to look for XDG config files in two different locations was
confusing. The XDG_DATA_HOME=temp-testpim/local/cache was also wrong,
because "cache" is for XDG_CACHE_HOME.
Now XDG_CONFIG/CACHE/DATA_HOME must be set to
temp-testpim/[config|cache|data], which is consistent with the usage
of private XDG dirs in test-dbus.py, and the directories are shared by
EDS and PIM Manager.
EDS keeps running throughout all tests. This must be taken into
account when cleaning temp-testpim. evolution-source-registry does not
cope with removal of its "sources" directory, so we have to keep that,
although we are removing its content. evolution-addressbook-factory
may continue to have an ESource open although its client *and* the
source are gone (should be fixed in EDS by tracking clients *and*
source removals!).
When that happens and we reuse a DB UUID,
evolution-addressbook-factory continues to use the deleted instance,
leading to IO errors. To avoid that inside a test run, we use
different UID prefices for each test, by embedding the test name. To
avoid it between test runs, kill evolution-addressbook-factory
first. When killing evolution-source-registry, use -9, because
otherwise it shuts down too slowly, which may lead to test failures
when it talks to the closing instance.
For the system address book we can't change the UID between tests, so
we must not delete it.
As with glib before, now GeeCollCXX also makes the parameter for declaring the
pointer ownership mandatory. The ensuing code analysis showed that in two
cases involving gee_multi_map_get() the transfer of ownership was not handled
correctly, leading to memory leaks.
Traditionally, glib messages were caught via LogRedirect.
Later, they got redirected by installing a handler. That change
disabled the message filtering for known harmless error messages
in LogRedirect, which caused messages from folks to show up
in TestCmdline D-Bus tests randomly, depending on the timing.
Reestablish the filtering by checking it in the glog callback.
To minimize changes, keep the error registry in LogRedirect.
It must be thread-safe now.
The D-Bus method reply may occur when the calling instance of
LocalTransportAgent is already gone. Therefore bind to a weak
pointer instead of this.
Seen when testing against Apple Calendar Server in the nightly
testing. It was a bit uncertain from the logs why it happened,
but it is clear that it can happen and thus the patch makes
sense even though it might not address the real problem.
Explicitly free the engine before releasing our lock on shutdown
signals. This prevents getting killed by a signal while we are still
freeing resources, which can take some time when a background thread
is involved (seen in some tests covering that).
The D-Bus TestHTTP tests occasionally failed because a message was resend
due to rounding issues. Be a bit (= 0.1 seconds) more conservative about
resending to avoid this.
Sometimes an extra "quiescent" signal was seen. It came from the
FolksAggregator before reporting any contacts. Not exactly sure
why that happens, but because it is not the responsibility of this
test to detect that, let's ignore it here.
We have to hard code this instead of always using it because ICU does
not properly fall back to non-phonebook defaults. Without phonebook as
collation, sorting was not done correctly in Germany.
Now also pay attention to case and punctuation. Previously these were
ignored for performance reasons. The reasoning that case doesn't
matter was wrong, it is useful as a tie breaker (which is what the
higher level does).
Full interleaving of Pinyin transliterations of Chinese names with
Western names can be done by doing an explicit Pinyin transliteration
as part of computing the sort keys.
This is done using ICU's Transliteration("Han-Latin"), which we have
to call directly because boost::locale does not expose that API.
We hard-code this behavior for all "zh" languages (as identified by
boost::locale), because by default, ICU would sort Pinyin separately
from Western names when using the "pinyin" collation.
The SyncPeer() result is derived from the sync statistics. To have
them available, the "sync done" signal must include the SyncReport.
Start and end of a sync could already be detected; "modified" signals
while a sync runs depends on a new signal inside the SyncContext when
switching from one cycle to the next and at the end of the last one.
Following the boost::instrusive_ptr example and making "add_ref =
true" the default in our CXX GLib and GObject wrappers led to some
memory leaks because it didn't enforce thinking about whether the
plain pointer is already owned by us or not.
It is better to use a mandatory enum value, ADD_REF and TRANSFER_REF,
and force explicit construction. Doing that revealed that the
assignment operator was implemented as constructing a CXX instance
with increased ref count and/or that in some places, a real leak was
caused by increasing the ref count unnecessarily.
Running under valgrind gave a false sense of security. Some of the
real leaks only showed up randomly in tests.
This mimics the behavior of boost smart pointers and is useful for
code where variables can be of either type, because "= NULL" is
not supported by our boost::intrusive_ptr wrapper.
Avoid referencing pathProps->second when the set of paths that
PROPFINDs returns is empty. Apparently this can happen in combination
with Calypso.
The stack backtrace sent via email looked like this:
Program received signal SIGSEGV, Segmentation fault.
0x4031a1a0 in std::_Rb_tree<std::string, std::pair<std::string const, std::string>, std::_Select1st<std::pair<std::string const, std::string> >, std::less<std::string>, std::allocator<std::pair<std::string const, std::string> > >::find(std::string const&) const () from /usr/lib/libsyncevolution.so.0
0x4031a1a0 <_ZNKSt8_Rb_treeISsSt4pairIKSsSsESt10_Select1stIS2_ESt4lessISsESaIS2_EE4findERS1_+60>: ldr r4, [r0, #-12]
(gdb) bt
from /usr/lib/syncevolution/backends/syncdav.so
from /usr/lib/syncevolution/backends/syncdav.so
from /usr/lib/libsyncevolution.so.0
Include the pid of the child process in all messages relating to it,
to help in cases where the same program is started multiple times.
Log something in the parent when destroying the fake API instance and
thus canceling the fake method call from the child, because that will
trigger a "lost connection" message in the child which was hard to
correlate with events in the server without those additional log
messages in the parent.
Sessions created via the Server.Connect() API were announced via
signals, but not activated. Therefore querying their status or
aborting them via D-Bus was not possible. Found while writing the
TestHTTP.testAbortThread D-Bus test.
Since the introduction of SuspendFlags, the ony remaining user
of the virtual aspect of checkForSuspend/checkForAbort() was the
testing code. By using the suspend/abort request mechanism in
SuspendFlags, it becomes possible to move checkForSuspend/Abort()
into SuspendFlags itself.
This will be useful to use it outside of a SyncContext member.
test-dbus.py now knows how to start syncevo-http-server. Therefore
it can test normal HTTP-based syncing as well as several scenarios
which fail or succeed with a slow server depending on the server's
ability to send SyncML messages while still initializing the storage.
To make the server slow, env variables are checked by the file
backend. It may matter whether open() or listAll() are slow, so test
both.
The tests expecting the 2 minute default must check whether the
feature is enabled at all in the binary that they are testing. If not,
the test cannot run. All other tests work, albeit somewhat unsafely
because they force the engine to run multithreaded when the engine was
compiled without mutex locking of global data structures.
We need to keep the glib event loop running while the main thread
waits for the background thread to finish. Otherwise code using
glib events (like EDS) may get stuck (observed with Sleep()
when using g_timeout_add() and synchronous calls in EDS 3.6).
We also need to watch for abort requests. When aborted, we give up
waiting for the thread and tell the engine to proceed. This will
eventually return control to us, where we can shut down.
When libsynthesis shuts down, it'll wait forever for the background
thread's shutdown. In this case we must wait and hope that the background
thread completes or aborts by itself.
HTTP SyncML clients give up after a certain timeout (SyncEvolution
after RetryDuration = 5 minutes by default, Nokia e51 after 15
minutes) when the server fails to respond.
This can happen with SyncEvolution as server when it uses a slow
storage with many items, for example via WebDAV. In the case of slow
session startup, multithreading is now used to run the storage
initializing in parallel to sending regular "keep-alive" SyncML
replies to the client.
By default, these replies are sent every 2 minutes. This can be
configured with another extensions of the SyncMLVersion property:
SyncMLVersion = REQUESTMAXTIME=5m
Other modes do not use multithreading by default, but it can be
enabled by setting REQUESTMAXTIME explicitly. It can be disabled
by setting the time to zero.
The new feature depends on a libsynthesis with multithreading enabled
and glib >= 2.32.0, which is necessary to make SyncEvolution itself
thread-safe. With an older glib, multithreading is disabled, but can
be enabled as a stop-gap measure by setting REQUESTMAXTIME explicitly.
Only one thread may handle events in the default context at any point
in time. If a second thread calls g_main_context_iteration() or
g_main_loop_run(), it blocks until the other thread releases ownership
of the context. In that case, the first thread may wake up because of
an event that the second thread waits for, in which case the second
thread may never wake up. See
https://mail.gnome.org/archives/gtk-list/2013-April/msg00040.html
This means that SyncEvolution can no longer rely on these functions
outside of the main thread. This affects Sleep() and the EDS backend.
As an interim solution, take over permanent ownership of the default
context in the main thread. This prevents fights over the ownership
when the main thread enters and leaves the main loop
repeatedly. Utility code using the main context must check for
ownership first and fall back to some other means when not the owner.
The assumption for the fallback is that the main thread will drive the
event loop, so polling with small delays for the expected status
change (like "view complete" in the EDS backend) is going to succeed
eventually.
A better solution would be to have one thread running the event loop
permanently and push all event handling into that thread. There is C++
utility code for such things in:
http://cxx-gtk-utils.sourceforge.net/2.0/index.html
See in particular the TaskManager class and its
make_task_when()/make_task_compose()/make_task_when_full() functions
for executing asynchronous results via a glib main loop, also the
Future::when() method and a number of other similar things in the
library.
Refactored the code into a new utility base class for use in other
tests.
Replace pipes with temporary files, using the same base name as the
traditional .syncevo.log and .dbus.log. They are numbered (because the
command line might be run multiple times per test) and use .out, .err,
or .outerr as suffix depending on what kind of output they
contain. The advantage is that the output gets recorded permanently.
Use that when waiting for command completion times out: in that case,
the content of the output file(s) gets added to the exception.
The subprocess handle returned by startCmdline() is extended with
information about output redirection that is then used by
finishCmdline(). This makes one parameter of finishCmdline()
redundant.
It means "the remote was initiated", not "we were initiated by the
remote side". Somewhat unfortunate choice of a variable name, but
leave it for now...
Use the monotonic system time. Timespec has sub-second resolution and
by using the monotonic time, we are independent of time corrections.
If retryDuration is smaller than retryInterval, then time out after
retryDuration instead of waiting once for retryInterval and then
checking retryDuration.
When the request timed out *exactly* after retryDuration or the next
resend would happen after the retryDuration, then give up immediately.
When the helper shuts down normally after the parent is done with it,
there was a race between handling the signal and the loss of connection,
leading to random "return 1" errors. This showed up in nightly testing
in particular in the test-dbus.py testConfigure.
The loss of connection at that point is not an error, so don't treat it
as one and simply return 0.
During normal operation, the helper's messages were never printed to any
output stream. They only went out via D-Bus. This was surprising when
debugging syncevo-dbus-server and it's interaction with syncevo-dbus-helper.
It's better to let the parent print the helper's output if the helper
doesn't do it itself, so that console output is the same with and without
SYNCEVOLUTION_DEBUG.
When a sync in syncevo-dbus-helper was aborted, the process did not
wait in main() for the server to acknowledge the D-Bus method
response. If the process quit to early, the parent did not get the
method response, leading to an "internal error" instead of "aborted"
as result of the sync.
The reason is that the acknowledgement was meant to be done via
SIGTERM, but that had already been sent and thus syncevo-dbus-helper
did not wait. Solved by using SIGURG for the acknowledgement.
In addition, the parent must not close its connection when sending
signals to the child, because the child may still need to send its
sync report and/or log output signals.
D-Bus objects in a process exist independently from a specific D-Bus
connection. They are only identified by their path. When syncevo-dbus-server
forked multiple syncevo-dbus-helper instances, it indirectly (in ForkExec)
created multiple callback objects for the childs "watch" functionality (a
pending method call that the parent never replies to). These method calls were
associated with the a random (oldest?) session instead of the current one.
This causes problems once an older session terminates and the callback object
destructs, because then the wrong children get a failure response, which they
treat as "connection lost = parent has terminated". Found while trying to fix
shutdown race conditions.
The solution is to generate a unique counter for each child and communicate
that counter to the child via a new env variable.
It can be useful to use additional signals like SIGURG or SIGIO
(typically not used elsewhere) for simple interprocess
communication. SuspendFlags now supports that by letting the process
catch more than just SIGINT and SIGTERM and recording which signals
were received.
activate() calls can happen when already active, for example when
syncevo-dbus-helper instantiates SyncContext, which activates signal
handling for the duration of the sync. This was not handled well
before, leaking previously allocated FDs and not restoring signal
handlers correctly. Because the process quit anyway soon, this did not
matter in practice.
Now the code explicitly checks for this and returns the existing Guard
in all following activate() calls, without changing the signal
handling.
Logging must be thread-safe, because the glib log callback may be
called from arbitrary threads. This becomes more important with EDS
3.8, because it shifts the execution of synchronous calls into
threads.
Thread-safe logging will also be required for running the Synthesis
engine multithreaded, to overlap SyncML client communication with
preparing the sources.
To achieve this, the core Logging module protects its global data with
a recursive mutex. A recursive mutes is used because logging calls
themselves may be recursive, so ensuring single-lock semantic would be
hard.
Ref-counted boost pointers are used to track usage of Logger
instances. This allows removal of an instance from the logging stack
while it may still be in use. Destruction then will be delayed until
the last user of the instance drops it. The instance itself must be
prepared to handle this.
The Logging mutex is available to users of the Logging module. Code
which holds the logging mutex should not lock any other mutex, to
avoid deadlocks. The new code is a bit fuzzy on that, because it calls
other modules (glib, Synthesis engine) while holding the mutex. If
that becomes a problem, then the mutex can be unlocked, at the risk of
leading to reordered log messages in different channels (see
ServerLogger).
Making all loggers follow the new rules uses different
approaches.
Loggers like the one in the local transport child which use a parent
logger and an additional ref-counted class like the D-Bus helper keep
a weak reference to the helper and lock it before use. If it is gone
already, the second logging part is skipped. This is the recommended
approach.
In cases where introducing ref-counting for the second class would
have been too intrusive (Server and SessionHelper), a fake
boost::shared_ptr without a destructor is used as an intermediate step
towards the recommended approach. To avoid race conditions while the
instance these fake pointers refer to destructs, an explicit
"remove()" method is necessary which must hold the Logging
mutex. Using the potentially removed pointer must do the same. Such
fake ref-counted Loggers cannot be used as parent logger of other
loggers, because then remove() would not be able to drop the last
reference to the fake boost::shared_ptr.
Loggers with fake boost::shared_ptr must keep a strong reference,
because no-one else does. The goal is to turn this into weak
references eventually.
LogDir must protect concurrent access to m_report and the Synthesis
engine.
The LogRedirectLogger assumes that it is still the active logger while
disabling itself. The remove() callback method will always be invoked
before removing a logger from the stack.
The method became obsolete when introducing fork+exec for local
syncing. Before that, the method was used to remove unsafe loggers in
the child's process. Now exec() does that for us.
Changing the process' name, even temporarily, is not thread-safe
and needs to be avoided. Instead pass the additional parameter
explicitly via the MessageOptions container.
Logger::formatLines() and LogStdout::write() are passed the process
name as "const std::string *" pointer, with Logger::getProcessName()
acting as fallback for NULL. Calling that is delayed as long as possible,
to avoid making the string copy when not necessary.
The previous implementation of formatLines() used a std::string
reference, but only used the content to determine whether to include
the current process name - probably not what was intended, but harmless
because either the empty string or the current name were passed.
The number of parameters for messagev has grown unreasonably, and more
parameters will be needed soon (explicit process name). Better store
the parameters in a struct and pass around a pointer to such a struct
instance. The code becomes shorter and new parameters can be passed
through intermediate functions without modifying them.
Passing an explicit Logger instance to the SE_LOG* macros was hardly
ever used and only made the macros more complex.
The only usage of it was in some backends, which then added a prefix
string automatically. The same effect can be achieved by passing
getDisplayName(). Exception handling no longer needs a Logger instance,
the prefix alone is enough.
The other intended usage, avoiding global variables for logging by
passing a logger known to the caller, was not possible at all.
To make prefix handling more flexible, it is now passed as a "const
std::string *" instead of a "const char *".
The new ThreadSupport.h provides C++ recursive and non-recursive
mutices for static and dynamic allocation, with RAII for unlocking
them automatically.
The current implementation depends on glib >= 2.32.0. Without it, the
C++ classes are dummies which do no real locking. On systems without
glib or a glib older than 2.32.0 (Debian Etch, Ubuntu Lucid 12.04),
thread support does not work because it depends on the revised glib
threading API from 2.32.0.
The call cannot succeed without an empty UID, so there's no point
even trying it. Worse, EDS 3.8 aborts the process when given
an empty UID:
[ERROR 00:00:00] GLib: g_variant_builder_end: assertion
`!GVSB(builder)->uniform_item_types || GVSB(builder)->prev_item_type != NULL
|| g_variant_type_is_definite (GVSB(builder)->type)' failed
[ERROR 00:00:00] GLib: g_variant_get_type: assertion `value != NULL' failed
[ERROR 00:00:00] GLib: g_variant_type_is_subtype_of: assertion `g_variant_type_check (type)' failed
[ERROR 00:00:00] GLib: g_variant_get_type_string: assertion `value != NULL' failed
[ERROR 00:00:00] GLib: g_variant_new: expected GVariant of type `a(ss)' but received value has type `(null)'
See https://bugzilla.gnome.org/show_bug.cgi?id=697705
In EDS 3.6.4, opening fails a lot more often with E_CLIENT_ERROR_BUSY.
We must retry until we succeed.
Seen in particular with testpim.py testFilterStartup. Clients are expected to
try the open call again. EDS >= 3.8 does that automatically.
The sorting got broken when the number of contacts exceeds the 3
digits reserved via the 03d formatting specifier. Now allow four
digits = up to 9999 contacts.
Binding _done() to self.start reads the *current* value of self.start
when _done() gets called. We need to bind to an inmuted local instance
instead to achieve the desired effect.
The destructor must be virtual, otherwise destructors in dervived
classes are not getting called when destructing via a downcasted
pointer. Should be relevant, but did not show up in valgrind?!
Klocwork complained about not checking the result. Depending on the
situation we now give up or use some kind of fallback (for example,
when logging).
SyncEvolution itself is not multithreaded, but utility libraries
might be, so better avoid the localtime() function and its global
state. May also be relevant when function A gets a tm * pointer,
calls function B which calls localtime() again, then A tries to
use its original value.
When realloc() returns NULL, the already allocated buffer remains
available. Previously we discarded (and leaked!) it. Now we track
whether that memory already contains parts of the next log message
and use that when realloc fails. The dgram will then get discarded
without reading the rest.
When reading from a stream, realloc will only be called once to
get the initial buffer. When that fails, we give up.
Klocwork warned about using a NULL starttime pointer in the case that
the transport is unknown. Not sure whether this can actually happen at the
moment, fix it anyway.
All of the jobs were allocated dynamically without freeing them
explicitly, instead relying on auto-deletion (enabled by default).
This behavior is surprising for SyncEvolution developers and Klocwork,
which expects explicit delete calls for each new. Better use
traditional RAII.
The advantage also is that each job gets deleted right away, instead
of waiting for another round of event processing, which may or may not
happen soon.
Qt has never been happy with the old code, reporting:
QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave.
This error message became visible again after some recent logging
changes and broke tests, so better avoid it by testing for D-Bus
with a private connection.
Klocwork correctly reported that the underlying implementation in
libsynthesis must not be called with NULL pointers. Therefore always
allow it to set the return values, even if our caller was not
interested.
The old code triggered a Klocwork warning about the missing self check
in the copy operator. This was not a real problem because of the
temporary instance from which m_error got taken, but that was not
obvious even to a human.
Better use standard coding practices:
- skip the operation when assigning to self
- reuse the set() operation
- return reference instead of copy (probably a bug!)
The code tried to transfer ownership of the GDBusMethodInfo and
GDBusSignalInfo arrays to the refcounted GDBusInterfaceInfo instance,
but that does not work because g_dbus_interface_info_unref() does not
free those arrays.
Let's simplify this so that all memory, including the GDBusInterfaceInfo
instance, is owned by DBusObjectHelper and kept around as long as the
object is active.
The stack-allocated GDBusInterfaceVTable happened to work in practice,
but the API description does not say whether
g_dbus_connection_register_object() makes a copy, so better keep that
around longer, too.
Finally, clean up checking for "is activated" to use only
m_connId. m_activated was redundant and the method resp. signal
pointers are now always non-NULL.
Without the explicit TYPE=INTERNET, email addresses sent to a Nokia
e51 were not shown by the phone and even got lost eventually (when
syncing back?).
This commit ensures that the type is set for all emails sent to any
Nokia phone, because there may be other phones which need it and
phones which don't, shouldn't mind. This was spot-checked with a N97
mini, which works fine with and without the INTERNET type.
This behavior can be disabled again for specific Nokia phones by
adding a remote rule which sets the addInternetEmail session variable
to FALSE again.
Non-Nokia phones can enable the feature in a similar way, by setting
the variable to TRUE.
Compilation takes a lot longer when using -O2 and the result
is less debugger friendly, so filter out the flag. This is
relevant when building a release where the flag is set for the
rest of the compilation.
Complaining about "ERROR SUMMARY" when using valgrind is a false
positive. Ignore that text by making it lower case before searching. Other
attempts based on regex matching somehow failed (UTF-8 encoding error?!).
Some peers have problems with meta data (CtCap, old Nokia phones) and
the sync mode extensions required for advertising the restart
capability (Oracle Beehive).
Because the problem occurs when SyncEvolution contacts the peers
before it gets the device information from the peer, dynamic rules
based on the peer identifiers cannot be used. Instead the local config
must already disable these extra features in advance.
The "SyncMLVersion" property gets extended for this. Instead of just
"SyncMLVersion = 1.0" (as before) it now becomes possible to say
"SyncMLVersion = 1.0, noctcap, norestart".
"noctcap" disables sending CtCap. "norestart" disables the sync mode
extensions and thus doing multiple sync cycles in the same session
(used between SyncEvolution instances in some cases to get client and
server into sync in one session).
Both keywords are case-insensitive. There's no error checking for
typos, so beware!
The "SyncMLVersion" property was chosen because it was already in use
for configuring SyncML compatibility aspects and adding a new property
would have been harder.
In the previous attempt (commit a0375e) setting the <syncmodeextensions>
option was only done on the client side, thus breaking multi-cycle
syncing with SyncEvolution as server. On the server side the option
shouldn't be needed (the server never sends the extensions unless
the client did first), but for symmetry and testing reasons it makes
sense to also offer the options there.
Previously, the database field was interpreted as a Collection ID. This adds
logic to allow the database to be interpreted as a folder path. The logic is:
1) If the database is an empty string, pass it through (this is the most
common case as it is interpreted as "use the default folder for the
source type").
2) If the database matches a Collection ID, use the ID (this is the same as
the previous behaviour).
3) If the database matches a folder path name, with an optional leading "/",
use the Collection ID for the matching folder.
4) Otherwise, force a FolderSync to get the latest folder changes from the
server and repeat steps 2 and 3
5) If still no match, throw an error.
Steps 2 and 3 are in the new function lookupFolder. The remaining logic has
been added to the open function. Note that the result is that m_folder (and
hence getFolder()) are always either empty or a Collection ID -- that is as
before so the sync logic itself is unchanged.
The command line swallowed errors thrown by the backend while listing
databases. Instead it just showed "<backend name>: backend failed". The goal
was to not distract users who accidentally access a non-functional backend.
But the result is that operations like --configure or --print-databases could
fail without giving the user any hint about the root cause of the issue.
Now the error explanation in all its gory details is included.
For example, not having activesyncd running leads to:
INFO] eas_contact: backend failed: fetching folder list:
GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name
org.meego.activesyncd was not provided by any .service files
And running activesyncd without the necessary gconf keys shows up as:
[INFO] eas_contact: backend failed: fetching folder list:
GDBus.Error:org.meego.activesyncd.Error.AccountNotFound: Failed to find
account [syncevolution@lists.intel.com]
Both of these happened when trying to use the "list database" ActiveSync
patches in the nightly test setup...
A new method, findCollections, fetches the folder list from the server and
creates two maps:
m_collections - store all the information about each collection (folder),
indexed by server collection ID
m_folderPaths - map full folder paths to collection IDs
getDatabases uses this data to returns the folder path, collection ID and a
flag indicating if the folder is the default for that type.
Note 1: getDatabases always asks activesyncd to update the folder list from the
server in order to return up to date information.
Note 2: this depends on a new libeasclient routine:
eas_sync_handler_get_folder_list
A quick-and-dirty solution for enabling phone number summaries when
creating contact databases in the PIM Manager: let the EDS backend
recognize the special UIDs used by the PIM Manager and then hard-code
the minimal set of summary fields and indexed fields which allow
executing the E_CONTACT_TEL, E_BOOK_QUERY_EQUALS_NATIONAL_PHONE_NUMBER
query quickly.
A proper solution would use a new EDS function for parsing ESource
defaults from a string and then providing these defaults to the
backend from the PIM Manager.
Also note that configuring the EDS system address book must be covered
elsewhere, because it wouldn't be correct for SyncEvolution as only
one of many clients to change the configuration of that.
To enable the special support, add the following section to
share/evolution-data-server-3.6/rw-sources/system-address-book.source:
[Backend Summary Setup]
SummaryFields=phone
IndexedFields=phone,phone
This patch adds new function calls to code shared by syncecal and syncebook,
so we have to add libebook-contacts to both to avoid link errors.
This reverts commit 2735273ec60b289c5ec2c49f3eacb9d7d04d5ea1.
With this patch, setting up ActiveSync fails with Google as server.
Needs further investigation.
Also note the explicit g_object_unref() and EASFolderUnref - these
should get replace with SE_GOBJECT_TYPE smart pointers.
This reverts commit 7327b23a4dd31abdc9596916743892402bcffe0c.
Depends on 273527 "ActiveSync: added getDatabases support for fetching
folder list" which has to be reverted.
The new ReplaceSearch is more flexible than RefineSearch. It can
handle both tightening the search and relaxing it. The downside of it
is the more expensive implementation (must check all contacts again,
then find minimal set of change signals to update view).
Previously, a search which had no filter set at all at the begining
could not be refined. This limitation of the implementation gets
removed by always using a FilteredView, even if the initial filter is
empty.
When the parent of a FilteredView emits signals, it is not necessarily
always in a consistent state and the FilteredView must not invoke
methods in the parent. Stressing the FilteredView by using it in tests
originally written for the FullView showed that the filling up a view
violated that rule and led to contacts being present multiple
times. Fixed by delaying the reading from the parent into either an
idle callback or the parent's quiescence signal processing, whichever
comes first.
Added priorities and proper tracking of "activated" state. The latter
depends on having the Timeout instance valid when the callback
returns, therefore the Server must delay the deletion of the instance
until the callback is done executing.
Priorities are needed to order different tasks in the PIM manager
correctly.
The "activated" state is something that the PIM manager needs to track
reliably, which had to be done by every callback. If a callback forgot
to do that, in the worst case this might have removed an entirely
unrelated source when the tag got reused in the meantime.
Previously, the database field was interpreted as a Collection ID. This adds
logic to allow the database to be interpreted as a folder path. The logic is:
1) If the database is an empty string, pass it through (this is the most
common case as it is interpreted as "use the default folder for the
source type").
2) If the database matches a Collection ID, use the ID (this is the same as
the previous behaviour).
3) If the database matches a folder path name, with an optional leading "/",
use the Collection ID for the matching folder.
4) Otherwise, force a FolderSync to get the latest folder changes from the
server and repeat steps 2 and 3
5) If still no match, throw an error.
Steps 2 and 3 are in the new function lookupFolder. The remaining logic has
been added to the open function. Note that the result is that m_folder (and
hence getFolder()) are always either empty or a Collection ID -- that is as
before so the sync logic itself is unchanged.
A new method, findCollections, fetches the folder list from the server and
creates two maps:
m_collections - store all the information about each collection (folder),
indexed by server collection ID
m_folderPaths - map full folder paths to collection IDs
getDatabases uses this data to returns the folder path, collection ID and a
flag indicating if the folder is the default for that type.
Note 1: getDatabases always asks activesyncd to update the folder list from the
server in order to return up to date information.
Note 2: this depends on a new libeasclient routine:
eas_sync_handler_get_folder_list
If phone number search is possible, then the direct search in EDS now
uses the more accurate E_BOOK_QUERY_EQUALS_NATIONAL_PHONE_NUMBER
comparison, with the E164 formatted caller ID as value to compare
against. That value includes the current country code.
For testing purposes, setting SYNCEVOLUTION_PIM_EDS_SUBSTRING forces
the usage of the traditional suffix match. This is used to test both
the old and new flavor of testFilterStartupRefine.
FilterStartupRefineSmart is the one which uses phone number matching.
When available, the pre-computed E164 number from EDS will be used
instead of doing one libphonebook parser run for each telephone number
while reading. Benchmarking showed that this parsing was the number
one hotspot, so this is a considerable improvement.
Google became even more strict about checking REV. Tests which
reused a UID after deleting the original item started to fail sometime
since middle of December 2012.
To fix this, tests must differentiate reuse of an item by adding a suffix
("-A", "-B", etc.). If CLIENT_TEST_UNIQUE_UID has a value >= 2, that suffix
will be used when mangling the input item. Otherwise the suffix is ignored
and nothing changes.
For testing Google, CLIENT_TEST_UNIQUE_UID=2 is used.
The backends had SYNCEVOLUTION_LIBS in their _DEPENDENCIES entries,
which is wrong because SYNCEVOLUTION_LIBS must include -lrt (which
can't be a dependency).
Fixed by depending on libsyncevolution.la directly.
The library might be used, for example when the PIM Manager is part of
the server, so better add it. Avoids hard linker errors on some
systems when it is used and not specified, which is better than given
one extraneous library that -as-needed can take care of.
The code which removed a photo from a contact did it by creating a
g_file_icon() with a NULL GFile. This is invalid and caused warnings
from glib. Eventually it must have passed NULL to folks, which is what
the new code does directly.
Don't let libphonenumber write to stdout. Instead redirect into
SyncEvolution logging and manage severity level. For example,
previously parsing errors of phone numbers were logged as [ERROR] by
libphonenumber. Now that appears as "phonenumber error: ...".
The previous macros assumed to be used inside the SyncEvo namespace
and had problems when used in a context where "Logger" was resolved to
something other than SyncEvo::Logger. Making that more explicit solved
that problem.
Use e_book_client_connect_direct_sync(), the official API, when
available. Support for e_book_client_new_direct() is still in the
code; it can be removed onces the 3.6 openismus-work branch adapts the
official API.
The new Bluez 5 API is the third supported API for doing PBAP
transfers. It gets checked first, then the PBAB backend falls back to
new-style obexd (file based, similar to Bluez 5, but not quite the
same) and finally old-style obexd (data transfer via D-Bus).
In contrast to previous APIs, Bluez 5 does not report the reason for a
failed PBAP transfer. SyncEvolution then throws a generic "transfer
failed" error with "reason unknown" as message.
When using more than 10 contacts (TESTPIM_TEST_ACTIVE_NUM), logging
and polling are turned towards causing less work by testpim.py itself.
TESTPIM_TEST_ACTIVE_RESPONSE can be used to give a maximum response
time in seconds, which gets checked by watchdog by calling
GetAllPeers().
This is relevant in two cases, data loaded before folks starts reading
from EDS (TESTPIM_TEST_ACTIVE_LOAD set) and folks getting change
notifications as SyncEvolution imports data into EDS (not set).
Related to FDO #60851. Currently SyncEvolution+Folks do not pass the
performance test.
The server must remain responsive at all times. The new Watchdog
class tests that by calling GetAllPeers() at regular intervals. It
can only be used in tests which do not block the event processing
in the Python script itself.
Once the watchdog runs, it must be removed when a test stops. The new
cleanup test member can be used to add cleanup code at runtime which
will be called when tearing down the test.
The D-Bus server has to respond to the entire request at once, which
makes it unresponsive. Needs to be avoided in the client, because
doing it on the server side is too complicated. The server would have
to start gathering results, but those results may become invalid again
in response to processing events before it can send them out.
Related to BGO #60851.
The \n in the testRead vCard was interpreted by Python instead of
being written into the file. Must mark the string as raw. Did not
affect the test itself, but the libebook vCard parser reported
warnings.
Track which changes need to be stored and how that gets processed. Was
useful for finding that modifying groups failed when the groups hadn't
actually changed.
Setting TESTPIM_TEST_ACTIVE_NUM to a value larger than 10 enables a
mode with less logging and more efficient event handling in
runUntil. Useful for performance testing, because otherwise testpim.py
itself consumes considerable CPU cycles.
Mark pending reads by adding the start time. Avoid requesting data
again which is still pending. Relevant when dealing with many contacts
in testActive, helps performance by avoiding redundant operations.
Sending DEBUG messages via the LogOutput signal is expensive and often
not necessary. Ideally LogOutput should only send data that someone is
interested in. The problem is that the SyncEvolution D-Bus API has no
way of specifying that and some clients, like dbus-monitor, would not be
able to use it.
Therefore this commit makes the default level of detail configurable
when syncevo-dbus-server is started, via a separate --dbus-verbosity
option.
This gets applied to output generated from syncevo-dbus-server itself
as well as output from sync sessions.
test-dbus.py exposes that via a new TEST_DBUS_QUIET option. The
default is to include DEBUG output. When TEST_DBUS_QUIET is set to a
non-empty string, only ERROR and INFO messages are sent.
That SetPeer() allows modifying and creating a config leads to race
conditions when multiple clients want to create a config. The new
CreateConfig() avoids that by atomically checking that a config does
not exist yet and creating it.
SetPeer() is still available for backwards compatibility. It continues
to be used for modifying an existing config in TestContacts.testSync
to check the effect of the logging settings.
Exposed as "location" -> (lat, long) in the D-Bus bindings.
Reading, writing and updating are supported. Depends on a folks
release which has the patch which adds FolksLocationDetails.
Default EDS test data now includes GEO. Several peers do not support
it, which gets ignored by synccompare.
Was lost earlier during syncing. Must be defined in field list and
vCard profile. Still not supported by PIM Manager, because folks doesn't
support it (see FDO #60373).
Constructing the GValues created additional references instead
of taking over ownership as intended. For refcounted GObject,
the solution is to pass the instance and let GValue take the
reference.
For other cases, ownership of a new instance is transfered to the
GValue using the new constructors with a boolean flag.
The existing constructors with native data pointers
always copied data or increased the reference counter.
It is useful to store the result of creation methods
directly in a GValue, so allow that with a second
variation of the constructors which explicitly take
an "addRef" parameter.
The test must be activated by compiling manager.cpp with
PIM_MANAGER_TEST_THREADING and then runs start() in a new
thread which gets created during startup.
The first attempt was buggy: the idle callback is always active and
prevents sleeping => 100% CPU load. It also seems to prevent proper
shutdown somehow.
This version is much simpler. It removes the task queue and the
manager's idle callback. Instead the non-main thread adds an idle
callback itself which gets run once. To have the main thread notice
the new idle callback, g_main_context_wakeup() is used, as before.
The reason for not using this approach before was that the glib
documentation only mentioned GAsyncQueue as one of its structures
which are thread-safe. A look at the code shows that GMainContext and
GMainLoop are also thread-safe.
This adds the infrastructure for shifting the work of the D-Bus
methods into the main thread if called by another thread, which may
happen when we add other bindings for them.
Work is shifted to the main thread via a GAsyncQueue +
g_main_context_wakeup(). The calling thread then blocks waiting on a
condition variable until signaled by the main thread. Results are
stored for the calling thread as part of the operation that it
provides. Exceptions in the main thread are caught+serialized as
string and deserialized+rethrown in the calling thread - a bit crude,
but should work and reuses existing code.
Google CalDAV currently sends invalid XML back when asked to include
CardDAV properties in a PROPFIND. This gets rejected in the XML
parser, which prevents syncing calendar data:
Neon error code 1: XML parse error at line 55: undeclared namespace prefix
The incorrect XML is this:
<D:propstat>
<D:status>HTTP/1.1 404 Not Found</D:status>
<D:prop>
...
<caldav:max-attendees-per-instance/>
<ns1:addressbook-home-set xmlns:ns1="urn:ietf:params:xml:ns:carddav"/>
==> <ns1:principal-address/>
<ns1:addressbook-description/>
<ns1:supported-address-data/>
<ns1:max-resource-size/>
</D:prop>
</D:propstat>
This was introduced on the server side sometime after December 12nd
2012 (tests run at that time showed a different response) and does not
affect SyncEvolution 1.2 because it did not yet ask for CardDAV
properties.
The workaround on the client side is to ask for only the properties
which are really needed.
Don't try to list non-existent directories, that causes an uncaught
Python exception. Happens when using testpim.py for the first time
in a clean directory.
The error message for an unexpected slow sync still mentioned
the old and obsolete "refresh-from-client/server" sync modes.
Better mention "refresh-from-local/remote".
This reverts commit 3a71a2bf53 and
commit a0375e0160.
Need to back out the workaround for broken peers because it breaks
SyncEvolution<->SyncEvolution syncing.
When compiled with PIM Manager, syncevo-dbus-server still should
shut down automatically when idle. Only when the PIM Manager was
started and has (or will have) a unified address book, then keep
the process running.
gcc warns about a "long int" vs. "unsigned int" mismatch when
compiling for x86. All of that is in debugging output printfs.
Cast the index_t into long int to avoid that.
Some gcc version did not like the usage of a smart pointer in a
boolean and expression (something about ambiguous conversion). Fixed
by making the conversion explicit, as NULL check.
Check for e_book_client_new_direct() in configure and use it if
found. This enables direct reading from the EDS file backend in the
direct searching of EDS address books.
Useful for moving the session directories to a temporary file system.
They are essentially just useful for debugging when used as part of
PIM Manager.
- "logdir" - a directory in which directories are created with
debug information about sync session
- "maxsessions" - number of sessions that are allowed to exist
after a sync (>= 0): 0 is special and means unlimited,
1 for just the latest, etc.;
old sessions are pruned heuristically (for example,
keep sessions where something changed instead of
some where nothing changed), so there is no hard
guarantee that the last n sessions are present.
Avoid writing config file changes to disk by enabling a new
"ephemeral" mode for syncing via the PIM Manager. This mode is enabled
via the sync mode parameter of the D-Bus API and from there passed
through to the SyncConfig and local sync helper. It cannot be enabled
in the config files (yet?).
In this mode, config file changes are not flushed resp. discarded
directly in the config nodes. This prevents writing to .ini files in
~/.config.
The "synthesis" binfile client files are still written, but they get
redirected into the session directory, which can (and should) be set
to a temp file system and get deleted again quickly.
Data dumps are turned off now in the configs created by the PIM
Manager.
As mentioned in FDO #56334, automatically starting folks
to get a unified address book together as soon as possible
makes sense. A new command line option now allows that:
-p, --start-pim
Activate the PIM Manager (= unified address book) immediately.
The D-Bus API was locked into the Manager class as private methods.
Make them public so that CreateContactManager() can call start(); the
other methods are public now for the sake of consistenty and because
they might be useful elsewhere.
The new default is to log error messages to syslog.
Command line options can increase (or reduce) verbosity
and choose whether stdout and/or syslog shall be active:
-d, --duration=seconds/'unlimited'
Shut down automatically when idle for this duration
-v, --verbosity=level
Choose amount of output, 0 = no output, 1 = errors,
2 = info, 3 = debug; default is 1.
-o, --stdout
Enable printing to stdout (result of operations) and
stderr (errors/info/debug).
-s, --no-syslog
Disable printing to syslog.
To get the same behavior as before when debugging via the Python
scripts, "--no-syslog --stdout --verbosity=3" is passed to
syncevo-dbus-server when started via Python.
m_startTime and m_processName were not used, can be removed. Instead
add tracking of the parent logger and forward messages to that, to
allow combining LoggerSyslog with some other logger (for example,
stdout or redirection).
Together with storing the sort order persistently, this allows
restarting the daemon and have it create the same unified address book
again.
The set is stored as comma, space or tab separated list of words in
the value of the "active" property in the
~/.config/syncevolution/pim-manager.ini file.
Switch to the new EDSRegistryLoader from libsyncevolution. This will
allow sharing the ESourceRegistry with the EDS backend.
We use the asynchronous loading method, to avoid blocking the
server. This requires turning the search() method into an asynchronous
method. The actual code runs once the ESourceRegistry is available,
whether it is needed or not (keeps the logic simpler and minimizes
code changes).
To avoid reindenting the old code, the try/catch block and error
checking for the callback was added in a separate intermediate
function which then calls the old code.
Add a class, EDSRegistryLoader, which owns the creation of
ESourceRegistry and notifies other components synchronously or
asynchronously when the instance is ready.
Using the UID as part of file names gets more problematic when
allowing colons. Remove that character from the API and enforce
the format in the source code.
Bypass folks while it still loads contacts and search for a phone
number directly in EDS. This is necessary to ensure prompt responses
for the caller ID lookup.
Done with a StreamingView which translates EContacts into
FolksIndividuals with the help of folks-eds = edsf.
Combining these intermediate results and switching to the final
results is done by a new MergeView class.
A quiescence signal is emitted after receiving the EDS results and
after folks is done. The first signal will be skipped when folks
finishes first. The second signal will always be send, even if
switching to folks did not change anything.
Together with an artificial delay before folks is considered done,
that approach make testing more reliable.
The PIM code and the EDS backend both need access to common classes
(like the smart pointers) and eventually will share one instance of
the ESourceRegistry.
Log "quiescence" signal. Avoid starting the initial search if "search
== None". Both will be needed for startup search testing.
While at it, use the "quiescence" signal to continue after the initial
search. Much faster than always waiting 5 seconds.
Move IndividualView, FullView and FilteredView into separate source
files. They are large enough to justify that and it'll avoid header
file dependency cycles.
Introduce a new View base class, which contains the "start"
functionality and a new name for debug messages.
The new StreamingView is based on it and will be used for reporting
results in an EDS query.
Avoid passing NULL IndividualCompare pointer to views. Instead make
the CompareFormattedName class available as the default compare
method.
While it is likely that there is a GError when adding a persona fails,
better check for it explicitly. The code would crash with a NULL
persona pointer.
Some finish functions must return more values than just the return
value of the finish function. They do that via return
parameters. Support that by first switching via the arity of the
finish function and then based on types of the parameters + return
value.
Up to five parameters for the finish function are now supported. To
minimize the number of different combinations, only two variantions
are supported once return values are involved:
- non-void return value, GError * as last parameter
- non-void return value, no GError
The special semantic of GError is preserved (owned by callback
wrapper). All other results must be freed by the C++ callback.
To have a sane ordering of results, return value and GError were
swapped, leading to the following ordering:
- return value of the _finish call, if non-void
- all return parameters of the _finish call, in the order
in which they appear there
- a GError is passed as "const GError *", with NULL if the
_finish function did not set an error; it does not have to
be freed.
This affects only one place, the handling of
folks_persona_store_add_persona_from_details in the PIM module.
A 'limit' search term with a number as parameter (formatted as string)
can be added to a 'phone' or 'any-contains' search term to truncate the
search results after a certain number of contacts. Example:
Search([['any-contains', 'Joe'], ['limit', '10']])
=> return the first 10 Joes.
As with any other search, the resulting view will be updated if
contact data changes.
The limit must not be changed in a RefineSearch(). A 'limit' term may
(but doesn't have to) be given. If it is given, its value must match
the value set when creating the search. This limitation simplifies the
implementation and its testing. The limitation could be removed if
there is sufficient demand.
Fix the order of changes to internal data structures and emitting
change signals so that the changes are completed first. That way, if
the change signals are hooked up to classes which call the view back,
those calls can be processed correctly.
The code for removing a contact did this incorrectly: it skipped
updating the indices when the removed contact wasn't included in the
filtered view, although indices in the parent changed.
Due to not mapping the local index in the view to the parent's index,
refining only worked in views where parent and child had the same
index for the contacts in the search view.
Make the tests more complex to trigger this bug and fix it by adding
the required indirection.
Also include the system address book in testing. May matter because
folks considers it the primary address book and thus has special
cases involving it.
This test creates three address books with (by default) 100 contacts
each and then selects different permutations of them. This found a bug
in folks where disabling address books had no effect (see
https://bugzilla.gnome.org/show_bug.cgi?id=689146, patch is pending
review).
Looking up by phone number spends most of its cycles in normalizing of
the phone numbers in the unified address book. Instead of doing that
work over and over again during the search, do it once while loading.
This has implications for several classes:
- IndividualData must be extended to hold the pre-computed values;
what kind of data can be stored there is currently hard-coded
in the LocaleFactory base class, to keep it simple and minimize
memory fragmentation.
- Internal contact changes and access must be based on IndividualData,
not FolksIndividual.
- FullView must know which LocaleFactory will pre-compute the data.
- Changing contacts may have no effect on sorting, but may lead
to different pre-computed data.
Looking up a phone number only once does not gain from this change, it
even gets slower (more memory intensive, less cache locality). Only
searching multiple times becomes faster.
Ultimately it would be best to store the normalized strings together
with the telephone number inside EDS when the contact gets
created.
Caching normalized telephone numbers will lead to two special cases:
- contact changed directly in EDS
- contact changed through folks
Cover both cases with extensions to testFilterLive and
testContactWrite.
The unit tests did not work because creating FolksIndividual
instances with attributes is not supported unless they come from
a specific folks backend.
Removing them in preparation of changing the API of
IndividualAggregator (pass LocaleFactory for pre-computing values).
folks and EDS do not support writing properties in parallel
(https://bugzilla.gnome.org/show_bug.cgi?id=652659). Must serialize
setting of modified properties.
Once that is done, we can also write photos ("is modified" check was
missing, now done with some code refactoring) and notes (which somehow
always end up being treated as "modified").
Set empty values if empty in D-Bus and non-empty in folks. Without
that step, properties that were removed via D-Bus would not get
removed inside folks.
We really must create empty sets. A NULL pointer is not accepted by
folks. Somehow it leads to internal "timeout" errors inside folks.
It can happen that Folks has non-empty roles details, but the
containes roles do not contain values that SyncEvolution exposes via
D-Bus. When checking whether "roles" must be added to the top-level
dictionary, the "non default" check must do a deep dive into the
content of the set, instead of just checking for non-empty (as for
other sets).
The problem is in the C++ D-Bus binding. If the method that gets bound
to D-Bus returns a value, that value is ignored in the signature:
int foo() => no out parameter
It works when the method is declared as having a retval:
void foo (int &result) => integer out parameter
This problem exists for both the libdbus and the GIO D-Bus
bindings. In SyncEvolution it affects methods like GetVersions().
The fix consists of always returning a type for the R return type in
the libdbus bindings. getReturn() returned "" because the type was not
a retval. In the GIO binding, the R type was not handled at all. Also
move some common code into a helper function.
This fixes the hotspot during populating the FullView content: moving
contacts around required copying IndividualData and thus copying
complex C++ structs and strings. Storing pointers and moving those
avoids that, with no lack of convenience thanks to boost::ptr_vector.
Reordering also becomes faster, because the intermediate copy only
needs to be of the pointers instead of the full content.
The new "checkpoints" split up the whole script run into pieces which
are timed separately, with duration printed to stdout. In addition,
tools like "perf" can be started for the duration of one phase.
Benchmarking is different from permanently reading and monitoring
changes. --read-all enables a single read operation once the initial
set of new contact IDs are known.
The signal was not sent reliably. Now it is guaranteed to be sent once
when a search has finished sending its initial results, and not
sooner.
This makes it possible to rely on the "Quiescence" signal when
checking whether the current data contains some contact or not.
When the unified address book (= FullView) was not running yet at the
time when a client wanted to search it, the unified address book was
not started and thus the search never returned results.
Will be tested as part of the upcoming testFilterQuiescence.
Running at full verbosity is much too slow when there many
contacts. Therefore adding:
--verbosity=VERBOSITY
Determine what is printed. 0 = only minimal progress
messages. 1 = also summary of view notifications. 2 =
also a one-line entry per contact. 3 = also a dump of
the contact. 4 = also debug output for the script
itself.
The --debug command line flag enables printing of the debug output
sent by SyncEvolution over D-Bus. To capture it in a timely manner,
the running method calls must be done asynchronously with a running
main loop.
This also solves the timeout problem more nicely and is closer to how
a real application should be written.
Use contact IDs in ReadContacts(). Finding the contacts in the view is
done with an (almost) brute force search: a linear search looks at all
contacts and compares against the requested ID. The only enhancement is
that searching starts at the index of the previous contact, which makes
it a bit more efficient when reading consecutively.
As first step, notifications and contact data include the new string
ID. It is a direct copy of the FolksIndividual ID. The next step is
to adapt the ReadContacts() implementation.
The change primarily affects notification buffering in the
Manager. Not only does it have to remember ranges, but also the
corresponding IDs.
The Python test now stores either a ID or a dictionary at each view
entry. This required changing all comparisons against None, the
previous value for "no data". The assertions in ContactsView now use
the unittest assertions because they produce nicer error output, and
these errors get logged via Python (to make them show up in the D-Bus
log together with the other logging of the notification callbacks).
As before, the ContactsView class is picked up as a test class with no
test members. Request that unittest skips it (will still show up in
testpim.py output, though).
