deltachat-core-rust icon indicating copy to clipboard operation
deltachat-core-rust copied to clipboard

Unify event emitter types for context and account manager

Open link2xt opened this issue 2 years ago • 4 comments

This is a proposal on top of #3421, breaking C API to unify event emitter types for contexts and account manager.

link2xt avatar Jun 11 '22 15:06 link2xt

#3421 changes only the Rust implementation, but keeps C API intact. I have an idea to break C API and make construction of event channel explicit, so library user will construct dc_event_emitter_t even before constructing the Context.

First advantage of this approach is that event channel construction never fails with an Error, so it's guaranteed that even if Context cannot be constructed, the errors can be logged via channel instead of using eprintln!. Logging to stderr via eprintln! is problematic in TUI applications, see discussion at #3419.

This is also useful for managing multiple contexts, especially in tests, and can be used both in test_utils.rs and likely python tests to aggregate all logs into a single sink. Instead of aggregating events by creating multiple threads and polling from multiple event emitters, it is possible to construct a single event channel and use a single event loop for multiple contexts.

And finally, it will fix #2280 by discouraging memory-unsafe way to stop event thread by deallocating dc_context_t and causing dc_get_next_event() to return NULL, as previously used by Python and I suspect still used by Node.js bindings, causing test segfaults. If a clone of the Sender is put into dc_event_emitter_t, user will not be able to close the sender side of the channel by destroying all contexts and dc_get_next_event() will never return NULL.

link2xt avatar Jun 11 '22 16:06 link2xt

I haven't added compatibility macros/wrappers, the only code that needs to be changed is Android and iOS clients. Node module is already changed in this PR.

link2xt avatar Jun 16 '22 17:06 link2xt

what if you kept dc_accounts_event_emitter_t and its functions but they are just aliases for dc_event_emitter_t. could we achieve the same without breaking the ffi?

flub avatar Jun 16 '22 18:06 flub

some #define should do the job:

#define dc_accounts_event_emitter_t dc_event_emitter_t
#define dc_accounts_get_next_event dc_get_next_event
#define dc_accounts_event_emitter_unref dc_event_emitter_unref

we could just add these lines to deltachat.h with a comment that we will remove that in the ~next version. this will give implementations some more time and remove the pressure, esp. when it comes to switching forth and back for whatever reason.

but - if desktop is already done i personally would also be fine without the defines, android/ios is usually easier to adapt anyway and i know what to do there :)

but there is also python , that probably needs adaotion as well? so, if these three defines make life easier ...

r10s avatar Jun 17 '22 08:06 r10s

I added defines for backwards compatibility. Python does not depend on them, but they will allow to build Android/iOS without changes.

link2xt avatar Aug 28 '22 21:08 link2xt