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

Tricky config framework semantics #201

Open
ckreibich opened this Issue Nov 1, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@ckreibich
Copy link
Contributor

ckreibich commented Nov 1, 2018

At least for new-comers to the framework it can be tricky to consistently trigger initialization/update code given a config option's value. Consider this:

module Foo;

export { option foo = F; }

function foo_handler(ID: string, foo_new: bool): bool
{
    print fmt("New foo: %s", foo_new);
    
    # Update stuff here based on foo's value
    # ...
    
    return foo_new;
}

event bro_init() {
    Option::set_change_handler("Foo::foo", foo_handler);
}

Here, foo_handler doesn't get called when you simply run the script without redefing Config::config_files. When you do redef it, the handler fires both when the config file sets foo to T, and when it sets it to F.

So you have to make sure that your initialization happens even when the handler doesn't get called, and you cannot write your handler assuming that the new value is actually different from the old one.

These arguably aren't bugs, but they do take getting used to. Perhaps worth coming up with some design patterns or additional documentation. If we do want to change functionality, we could consider adding a mechanism that lets the user specify that they'd like to see the change handler triggered once at startup, and that they'd like to skip repeat invocations with the same value.

@0xxon

This comment has been minimized.

Copy link
Member

0xxon commented Nov 4, 2018

Just to state the obvious - you are obviously right with your example.

The reason that change handlers do get called one time when a configuration file was read, even if there was no change is that it is internally a bit... challenging to figure out if there has been a change between an old value and a new one and I thought it is just safer to get people used to the fact that change-handlers can fire several times.

There is a whole bunch of reasons for that. While for a simple example (e.g. having a boolean) not calling the change handler seems easy, things get a bit more complicated when there are complex datatypes (we don't really have an easy way to compare equality for some of them). Furthermore - some change-handlers will reformat the input data. So the current semantics are "the change handler is called whenever the respective configuration data reader deems it necessary; in case of configuration files they are called when the ascii-representation of the configuration value in the file changes".

In any case - I will add that explanation.

As to the second part - I am in principle happy to add something that calls the change handler once on Bro init with the initialization value. I think this behavior should be opt-in though; it might be only me but I actually like the way that it currently works (you have one kind-of-reasonable value defined on startup and one when things change).

Do you have an idea on the syntax you would like for an opt-in? Or would you be ok with the current behavior just being explained in documentation? I actually don't have especially strong feelings in any direction.

@rsmmr

This comment has been minimized.

Copy link
Member

rsmmr commented Nov 5, 2018

For the opt-in, could just be an additional argument to set_event_handler(), and fire immediately at that point?

@0xxon

This comment has been minimized.

Copy link
Member

0xxon commented Nov 7, 2018

We could not really fire immediately at that point... there might be priorities to take into account.

I think we in that case basically should wait till the end of bro_init (till hopefully the handlers have been populated) and then call them in the normal priority order.

Thinking more about it - I am actually a tiny bit tempted to just keep the current semantics and just expand the documentation. But - that is just my feeling on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment