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

filterx: cache_json_file() inotify based automatic reload #517

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bshifter
Copy link
Member

Due to multiple workers, the original frozen objects must be retained until all workers have finished. Since there is currently no way to track the workers' lifecycle, we call reload, which unfreezes and releases all previously stored elements to prevent high memory usage. This is a suboptimal solution and likely temporary. The upcoming RCU feature will provide a better way to handle this issue. However, the soon-to-be-released native dictionary implementation will also support the optimal solution.

@bshifter bshifter force-pushed the fx-cache-file-json branch 8 times, most recently from ea1fd85 to 6656632 Compare February 19, 2025 08:00
@MrAnno MrAnno self-requested a review February 19, 2025 10:24
lib/mainloop.c Outdated
@@ -167,7 +167,7 @@ struct _MainLoop

MainLoopOptions *options;
ControlServer *control_server;
CfgMonitor *cfg_monitor;
FileMonitor *file_monitor;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the instance name cfg_monitor in this unit?

void cfg_monitor_start(CfgMonitor *self);
void cfg_monitor_stop(CfgMonitor *self);
void file_monitor_start(FileMonitor *self);
void file_monitor_start_silent(FileMonitor *self);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not look like a "silent" version of start, it's more like whether you want an "immediate" check or not.

What do you think of start_immediate() or start_and_check()?
Don't you need a bool return value for the event callback? In that case start_and_check() could return that as well.

if (!self->frozen_objects)
return;
self->frozen_objects_history[self->history_index] = self->frozen_objects;
self->history_index++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an assert not to overflow the buffer in case the cleanup mechanism fails or someone refactors the guarantees away?

main_loop_assert_main_thread();
if (self->history_index >= FROZEN_OBJECTS_HISTORY_SIZE)
{
main_loop_reload_config(main_loop_get_instance());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move out this condition and the reload call to _file_monitor_callback (the name _load_json_file_version does not suggest that such intrusive side-effect might happen)?
Once it is done, can we add a comment to _file_monitor_callback() that this function may reload the configuration, so nothing complicated should be done with config-related references on the caller side?

@MrAnno
Copy link
Member

MrAnno commented Feb 25, 2025

Nice work! :)

- Support configurable filename
- Add support for DELETE event
- Renamed to file-monitor

Signed-off-by: shifter <[email protected]>
Due to multiple workers, the original frozen objects must be retained until all workers have finished. Since there is currently no way to track the workers' lifecycle, we call reload, which unfreezes and releases all previously stored elements to prevent high memory usage. This is a suboptimal solution and likely temporary. The upcoming RCU feature will provide a better way to handle this issue. However, the soon-to-be-released native dictionary implementation will also support the optimal solution.

Signed-off-by: shifter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants