Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send all Commits to the Commit Monitor #253

Closed
joepio opened this issue Dec 21, 2021 · 2 comments · Fixed by #381
Closed

Send all Commits to the Commit Monitor #253

joepio opened this issue Dec 21, 2021 · 2 comments · Fixed by #381
Labels
help wanted Extra attention is needed plugin Should probably be an Atomic Plugin

Comments

@joepio
Copy link
Member

joepio commented Dec 21, 2021

Whenever a user POSTs a commit to /commits, the handler notifies the Commit Monitor:

    // notify all webhook subscribers
    appstate
        .commit_monitor
        .do_send(crate::actor_messages::CommitMessage {
            subject: incoming_commit.subject,
            commit_response,
        });

This means that commits that are created through some other method (e.g. when a user uploads a File), these never reach the commit monitor, and are therefore never sent to the client using webhooks. For now, this is not that big of a deal, but I expect more and more ways of creating commits to appear - and not all of them will use the server!

So what should I do to fix this?

Add the above code to the /upload endpoint, deal with the problem later

The lazy approach. Fixes my issue now, but doesn't really work in the long term.

The Store (DB) stores one or more onCommit handlers, which are called by the Store.

This probably means that the Store struct should contain a Vec of functions, which are called when commit_opts is called. This function takes the output of commit_opts, and does something. On initializing the server, we could pass the above function to the Store.

Thoughts on extensibility / plugin system

I want other developers to one day do some logic upon handling a commit. It's a very useful callback / middleware. But how should this system work? I think that storing the registered functions in the Store might be OK, but I also think that a system like actix that uses actors might be more scalable, as it enables async workflows. Perhaps the one adding the function should determine whether it's a function that needs waiting (such as 'modify the commit'), or one that should be fire and forget (such as 'update some index')

@joepio
Copy link
Member Author

joepio commented Dec 21, 2021

I'm trying to store some functions in a struct:

type AfterCommit = fn(crate::commit::CommitResponse);

#[derive(Default, Clone)]
pub struct Handlers {
    pub after_commit: Vec<AfterCommit>,
}

impl Handlers {
    pub fn register_after_commit(&mut self, fun: AfterCommit) {
        self.after_commit.push(fun);
    }
}

But when I try to add a function:

    // closures can only be coerced to `fn` types if they do not capture any variables
    let handle_commit = |commit_response: atomic_lib::commit::CommitResponse| {
        commit_monitor.do_send(crate::actor_messages::CommitMessage {
            subject: commit_response.commit.get_subject().clone(),
            commit_response,
        })
    };

    store.handlers.register_after_commit(handle_commit);

So I'll probably have to store the functions in the struct in some other manner.

I could try with generics, which looks a bit like this:

pub struct Handlers<T>
where
    T: Fn(crate::commit::CommitResponse),
{
    /// Is called after succesfully applying a Commit
    pub after_commit: Vec<T>,
}

impl<T> Handlers<T> {
    /// Is called after succesfully applying a Commit
    pub fn register_after_commit(&mut self, fun: T) {
        self.after_commit.push(fun);
    }
}

But that means that the struct creating this should also provide information about this T value, which makes the Store API feel very messy.

And putting it in a Box doesn't work:

// the trait bound `dyn std::ops::Fn(commit::CommitResponse): std::clone::Clone` is not satisfied required because of the requirements on the impl of `std::clone::Clone` for `std::boxed::Box<dyn std::ops::Fn(commit::CommitResponse)>`

#[derive(Default, Clone)]
pub struct Handlers {
    pub after_commit: Vec<Box<AfterCommit>>,
}

impl Handlers {
    pub fn register_after_commit(&mut self, fun: AfterCommit) {
        self.after_commit.push(Box::new(fun));
    }
}

So let's try Arc, which can be cloned.
This seems to work pretty well, except for that some time later in the code, I try to put its parent struct Db into a Mutex, which requires Send:

#[derive(Default, Clone)]
pub struct Handlers {
    pub after_commit: Vec<Arc<AfterCommit>>,
}

impl Handlers {
    pub fn register_after_commit(&mut self, fun: Arc<AfterCommit>) {
        self.after_commit.push(fun);
    }
}

// (dyn std::ops::Fn(commit::CommitResponse) + 'static)` cannot be sent between threads safely
// the trait `std::marker::Send` is not implemented for `(dyn std::ops::Fn(commit::CommitResponse) + 'static)`
// required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<(dyn std::ops::Fn(commit::CommitResponse) + 'static)>`
use lazy_static::lazy_static; // 1.4.0
use std::sync::Mutex;
lazy_static! {
    pub static ref DB: Mutex<Db> = Mutex::new(init("shared"));
}

Anybody reading this and knows how to help... I'd be grateful!

@joepio joepio added help wanted Extra attention is needed plugin Should probably be an Atomic Plugin labels Dec 21, 2021
@joepio
Copy link
Member Author

joepio commented Apr 15, 2022

After reading about Send, Sync, Arc, Box and some more... I finally did it :')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed plugin Should probably be an Atomic Plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant