Commit 3dc2aa65 authored by rhi's avatar rhi 🐑
Browse files

Sync algorithm bug fixes/improvements

- Collection sync: don't save new sync state before downloading is finished
- throw exception when waiting for completion times out
- always use multi-get, even for single vCards/iCalendars
parent 43f4d9c0
Pipeline #123763615 passed with stages
in 9 minutes and 38 seconds
......@@ -13,7 +13,6 @@ import android.content.Context
import android.content.SyncResult
import android.os.Bundle
import at.bitfire.dav4jvm.DavCalendar
import at.bitfire.dav4jvm.DavResource
import at.bitfire.dav4jvm.DavResponseCallback
import at.bitfire.dav4jvm.Response
import at.bitfire.dav4jvm.exception.DavException
......@@ -113,41 +112,32 @@ class CalendarSyncManager(
override fun downloadRemote(bunch: List<HttpUrl>) {
Logger.log.info("Downloading ${bunch.size} iCalendars: $bunch")
if (bunch.size == 1) {
val remote = bunch.first()
// only one contact, use GET
useRemote(DavResource(httpClient.okHttpClient, remote)) { resource ->
resource.get(DavCalendar.MIME_ICALENDAR.toString()) { response ->
// CalDAV servers MUST return ETag on GET [https://tools.ietf.org/html/rfc4791#section-5.3.4]
val eTag = response.header("ETag")?.let { GetETag(it).eTag }
?: throw DavException("Received CalDAV GET response without ETag")
response.body()!!.use {
processVEvent(resource.fileName(), eTag, it.charStream())
/* Always use calendar-multi-get to get the iCalendars.
We don't use single GETs anymore, because then there are two methods, and in case of a bad
server (like iCloud) syncing could fail when there's only one changed resource
and succeed when there are multiple changed resources (or vice versa). This is intransparent
to the user. Also, when there's only one method, there's less code to maintain and there
are less possibilities for errors.
*/
useRemoteCollection {
it.multiget(bunch) { response, _ ->
useRemote(response) {
if (!response.isSuccess()) {
Logger.log.warning("Received non-successful multiget response for ${response.href}")
return@useRemote
}
}
}
} else
// multiple iCalendars, use calendar-multi-get
useRemoteCollection {
it.multiget(bunch) { response, _ ->
useRemote(response) {
if (!response.isSuccess()) {
Logger.log.warning("Received non-successful multiget response for ${response.href}")
return@useRemote
}
val eTag = response[GetETag::class.java]?.eTag
?: throw DavException("Received multi-get response without ETag")
val eTag = response[GetETag::class.java]?.eTag
?: throw DavException("Received multi-get response without ETag")
val calendarData = response[CalendarData::class.java]
val iCal = calendarData?.iCalendar
?: throw DavException("Received multi-get response without address data")
val calendarData = response[CalendarData::class.java]
val iCal = calendarData?.iCalendar
?: throw DavException("Received multi-get response without address data")
processVEvent(DavUtils.lastSegmentOfUrl(response.href), eTag, StringReader(iCal))
}
processVEvent(DavUtils.lastSegmentOfUrl(response.href), eTag, StringReader(iCal))
}
}
}
}
override fun postProcess() {
......
......@@ -14,7 +14,6 @@ import android.os.Build
import android.os.Bundle
import android.provider.ContactsContract.Groups
import at.bitfire.dav4jvm.DavAddressBook
import at.bitfire.dav4jvm.DavResource
import at.bitfire.dav4jvm.DavResponseCallback
import at.bitfire.dav4jvm.Response
import at.bitfire.dav4jvm.exception.DavException
......@@ -269,42 +268,33 @@ class ContactsSyncManager(
}
override fun downloadRemote(bunch: List<HttpUrl>) {
Logger.log.info("Downloading ${bunch.size} vCards: $bunch")
if (bunch.size == 1) {
val remote = bunch.first()
// only one contact, use GET
useRemote(DavResource(httpClient.okHttpClient, remote)) { resource ->
resource.get("text/vcard;version=4.0, text/vcard;charset=utf-8;q=0.8, text/vcard;q=0.5") { response ->
// CardDAV servers MUST return ETag on GET [https://tools.ietf.org/html/rfc6352#section-6.3.2.3]
val eTag = response.header("ETag")?.let { GetETag(it).eTag }
?: throw DavException("Received CardDAV GET response without ETag")
response.body()!!.use {
processVCard(resource.fileName(), eTag, it.charStream(), resourceDownloader)
Logger.log.info("Downloading ${bunch.size} vCard(s): $bunch")
/* Always use addressbook-multi-get to get the vCards.
We don't use single GETs anymore, because then there are two methods, and in case of a bad
server (like iCloud) syncing could fail when there's only one changed resource
and succeed when there are multiple changed resources (or vice versa). This is intransparent
to the user. Also, when there's only one method, there's less code to maintain and there
are less possibilities for errors.
*/
useRemoteCollection {
it.multiget(bunch, hasVCard4) { response, _ ->
useRemote(response) {
if (!response.isSuccess()) {
Logger.log.warning("Received non-successful multiget response for ${response.href}")
return@useRemote
}
}
}
} else
// multiple vCards, use addressbook-multi-get
useRemoteCollection {
it.multiget(bunch, hasVCard4) { response, _ ->
useRemote(response) {
if (!response.isSuccess()) {
Logger.log.warning("Received non-successful multiget response for ${response.href}")
return@useRemote
}
val eTag = response[GetETag::class.java]?.eTag
?: throw DavException("Received multi-get response without ETag")
val eTag = response[GetETag::class.java]?.eTag
?: throw DavException("Received multi-get response without ETag")
val addressData = response[AddressData::class.java]
val vCard = addressData?.vCard
?: throw DavException("Received multi-get response without address data")
val addressData = response[AddressData::class.java]
val vCard = addressData?.vCard
?: throw DavException("Received multi-get response without address data")
processVCard(DavUtils.lastSegmentOfUrl(response.href), eTag, StringReader(vCard), resourceDownloader)
}
processVCard(DavUtils.lastSegmentOfUrl(response.href), eTag, StringReader(vCard), resourceDownloader)
}
}
}
}
override fun postProcess() {
......
......@@ -199,11 +199,11 @@ abstract class SyncManager<ResourceType: LocalResource<*>, out CollectionType: L
} else
throw e
}
Logger.log.log(Level.INFO, "Saving sync state", syncState)
localCollection.lastSyncState = syncState
}
Logger.log.log(Level.INFO, "Saving sync state", syncState)
localCollection.lastSyncState = syncState
Logger.log.info("Server has further changes: $furtherChanges")
} while(furtherChanges)
......@@ -436,6 +436,12 @@ abstract class SyncManager<ResourceType: LocalResource<*>, out CollectionType: L
Logger.log.info("Number of local non-dirty entries: $number")
}
/**
* Calls a callback to list remote resources. All resources from the returned
* list are downloaded and processed.
*
* @param listRemote function to list remote resources (for instance, all since a certain sync-token)
*/
protected open fun syncRemote(listRemote: (DavResponseCallback) -> Unit) {
// results must be processed in main thread because exceptions must be thrown in main
// thread, so that they can be catched by SyncManager
......@@ -532,7 +538,8 @@ abstract class SyncManager<ResourceType: LocalResource<*>, out CollectionType: L
// process remaining responses
processor.shutdown()
processor.awaitTermination(5, TimeUnit.MINUTES)
if (!processor.awaitTermination(5, TimeUnit.MINUTES))
throw TimeoutException("Processing the remote resource list took too long")
// download remaining resources
if (toDownload.isNotEmpty())
......@@ -540,7 +547,8 @@ abstract class SyncManager<ResourceType: LocalResource<*>, out CollectionType: L
// signal end of queue and wait for download thread
downloader.shutdown()
downloader.awaitTermination(5, TimeUnit.MINUTES)
if (!downloader.awaitTermination(5, TimeUnit.MINUTES))
throw TimeoutException("Downloading and processing the remote resources took too long")
// check remaining results for exceptions
checkResults(results)
......
......@@ -9,7 +9,7 @@
buildscript {
ext.versions = [
dokka: '0.10.0',
kotlin: '1.3.61',
kotlin: '1.3.70',
okhttp: '3.12.10'
]
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment