A great portion of this past month was dedicated to internal improvements and several small bug-fixes. I much appreciate testers early adopters providing feedback when things don’t quite work as expected.

The only new feature is support for digest authentication, which has been tested using baikal. This has been a long requested feature and, for some folks, a blocker for moving from vdirsyncer.

Digest authentication requires doing a pre-flight request to the server. The response for this request includes a nounce, which is used to for the authentication in the “real” request which is sent as a follow-up. This nounce can be re-used by including a counter which increments for each request. Regrettably, this counter is a problem when performing concurrent requests: even if request 1 and request 2 are dispatched in order, there’s no guarantee that they’ll reach the server in the same order. If request 2 arrives first, then request 1 gets rejected (because it moves this counter backwards). Dealing with this quirk reliably seems unfeasible, so I’ve taken the simpler path and simply used an additional pre-flight request (and a new nonuce) for every request made. I’m not thrilled by this due to the network overhead, but it seems to be the price for digest auth.

The logic for digest authentication lives in the pimsync repository. In future, once Rust’s hyper library (the library for doing HTTP) stabilises its client interface, I intend to submit it upstream to hyper-util (or publish it in a stand-alone library if undesirable here).

Unknown configuration directives are now rejected. I implemented this while re-writing the existing configuration parsing logic. The existing parser read the configuration into a dict-like interface, from where directives and their parameters would be extracted. Ensuring that this kind of interface had no superfluous directives was complex and brittle, adding to an already messy implementation. I’d been wanting to clean up this area of the code for a while, and I’m quite happy with the result. On one hand, the code for parsing the configuration is now almost twice the amount of lines, but on the other hand, it’s much cleaner code, easy to read and reason about. One portion is just structs and enums which model the actual configuration file, another portion handles parsing it, and another resolving dynamic parameters (e.g.: executing commands to obtain passwords).

I still have issues accepting a change that increments the amount of code so much. Simpler, cleaner and more robust code is definitely a valid trade-off.

Internal non-functional changes

[permalink]

As mentioned above, I implemented several changes with no obvious user-facing impact but are a solid improvement for development and API usage. Reminder: most of the logic underneath pimsync is in re-usable libraries.

libdav provides WebDAV, CalDAV and CardDAV clients which can use any underlying HTTP client. This flexibility is implemented via generics, making it have zero runtime cost. This had a complexity cost: the generic bounds needed to be repeated on each use:

impl<C> CalDavClient<C>
where
    C: Service<http::Request<String>, Response = Response<Incoming>> + Sync + Send,
    <C as Service<http::Request<String>>>::Error: std::error::Error + Send + Sync,
{

That’s terrible, and was repeated dozens of times thought the codebase. To make things worse, any consumer for the libdav library needed to repeat those trait bounds too.

Using a type alias isn’t feasible for this kind of use of generics. And using a custom trait seemed like an issue: if a downstream application wants to use a third-party HTTP client, they need to implement this custom trait for the client, but can’t due to the orphan rule. Using a newtype wrapper would be a feasible workaround, but now we’re just piling too much complexity and abstractions.

It turns out I can use a custom trait plus a blanket implementation. That is: WebDAVClient is generic over a type implementing HttpClient, and I also provide a blanket implementation of HttpClient for anything which implements the above trait, Service<http::Request<String>, Response = Response<Incoming>> ….

This means that any type which already implement Service<http::Request… is immediately usable without consumers needing to provide an implementation of HttpClient directly. This enabled the removal of a lot of repetitive trait bounds across libdav and vstorage. Users of libdav can also implement a similar simplification.

I’m not sure if there’s a specific name for this pattern in Rust (I haven’t found references to it, frankly), but it’s a pretty neat one which really trims away a lot of complexity.

I also refactored libdav to use the http::Request type directly when generating requests. For historical reasons, it was using a bespoke PreparedRequest type which is no longer required.

On the vstorage side, I simplified the Plan implementation substantially. The current design had been tweaked gradually while improving the original implementation, and it was due time to take a step back and clean up some edges that are the result of this gradual process. A lot of code was dropped and other bits are much cleaner for this.

The synchronisation loop is now simplified. Previously, initiating a sync task would run an internal loop, which called an on_error callback with any non-fatal errors. pimsync prints the errors to stdout, but a UI could potentially render them in some other way. The new design is much simpler: the caller gets to drive the sync loop themselves, calling plan.next() to get the next operation and executor.execute_operation(op) to execute it. This is just a couple of more lines at call sites, but is far more “normal” code, and enables handling errors in a much more natural way.

The monitoring interface has changed entirely. Previously storages would use an internal interval timer to trigger synchronisations every fixed period. This was key for storages which don’t yet implement live monitoring. The timer interval required that every storage monitoring implementation interleave it with regular events, making both the internal implementation, handling failure scenarios and the API all messy. I removed the interval timer from the storage monitors: the “sync every fixed period” behaviour is now implemented externally by just using a regular timer in the event loop.

I intend to work on the BSD/kqueue monitoring implementation soon, and this last set of changes will make that that work much simpler and the result more robust.