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.

Sign up for GitHub

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

Jump to bottom

Fix race condition in FilterX simple funcs and perf improvements #257

Merged
merged 8 commits into from
Aug 29, 2024
Merged

Fix race condition in FilterX simple funcs and perf improvements #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a34c2c7
Revert "filterx-expr: make ref_cnt atomic"
MrAnno Aug 23, 2024
ebb68c9
filterx-expr: assert on non-thread-safe ref()/unref()
MrAnno Aug 23, 2024
5ad12e2
filterx-function: extract non-refed _args_get_expr()
MrAnno Aug 23, 2024
7b95d9a
filterx-function: avoid unnecessary ref-counts in function_args_get_*()
MrAnno Aug 23, 2024
e6677f3
filterx-function: add assert for non-thread-safe function
MrAnno Aug 23, 2024
f9ae977
filterx-function: remove function_args_get_*_object()
MrAnno Aug 23, 2024
9fc2316
filterx-function: check arguments in constructor
MrAnno Aug 23, 2024
e287dc2
filterx-metrics: do not call racy expression unref() from non-main th…
MrAnno Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 96 additions & 87 deletions lib/filterx/expr-function.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/*
* Copyright (c) 2024 Axoflow
* Copyright (c) 2023 Balázs Scheidler <[email protected]>
* Copyright (c) 2023 shifter
*
Expand Down Expand Up @@ -32,6 +33,7 @@
#include "filterx/object-null.h"
#include "plugin.h"
#include "cfg.h"
#include "mainloop.h"

GQuark
filterx_function_error_quark(void)
Expand All @@ -42,10 +44,16 @@ filterx_function_error_quark(void)
typedef struct _FilterXSimpleFunction
{
FilterXFunction super;
FilterXFunctionArgs *args;
GPtrArray *args;
FilterXSimpleFunctionProto function_proto;
} FilterXSimpleFunction;

struct _FilterXFunctionArgs
{
GPtrArray *positional_args;
GHashTable *named_args;
};

void
filterx_simple_function_argument_error(FilterXExpr *s, gchar *error_info, gboolean free_info)
{
Expand All @@ -54,31 +62,44 @@ filterx_simple_function_argument_error(FilterXExpr *s, gchar *error_info, gboole
filterx_eval_push_error_info(self ? self->super.function_name : "n/a", s, error_info, free_info);
}

static inline FilterXExpr *
_args_get_expr(FilterXFunctionArgs *self, guint64 index)
{
/* 'retrieved' is not thread-safe */
main_loop_assert_main_thread();

if (self->positional_args->len <= index)
return NULL;

FilterXFunctionArg *arg = g_ptr_array_index(self->positional_args, index);
arg->retrieved = TRUE;
return (FilterXExpr *) arg->value;
}

static FilterXObject *
_get_arg_object(FilterXSimpleFunction *self, guint64 index)
{
FilterXExpr *expr = g_ptr_array_index(self->args, index);
if (!expr)
return NULL;

return filterx_expr_eval(expr);
}

static GPtrArray *
_simple_function_eval_args(FilterXSimpleFunction *self)
{
guint64 len = filterx_function_args_len(self->args);
GPtrArray *res = g_ptr_array_new_full(self->args->len, (GDestroyNotify) filterx_object_unref);

GPtrArray *res = g_ptr_array_new_full(len, (GDestroyNotify) filterx_object_unref);

for (guint64 i = 0; i < len; i++)
for (guint64 i = 0; i < self->args->len; i++)
{
FilterXObject *obj = filterx_function_args_get_object(self->args, i);
FilterXObject *obj = _get_arg_object(self, i);
if (obj == NULL)
goto error;

g_ptr_array_add(res, obj);
}

GError *error = NULL;
if (!filterx_function_args_check(self->args, &error))
{
filterx_simple_function_argument_error(&self->super.super, error->message, TRUE);
error->message = NULL;
g_clear_error(&error);
goto error;
}

return res;

error:
Expand All @@ -93,7 +114,7 @@ _simple_eval(FilterXExpr *s)

GPtrArray *args = NULL;

if (!filterx_function_args_empty(self->args))
if (self->args->len)
{
args = _simple_function_eval_args(self);
if (!args)
Expand All @@ -115,23 +136,53 @@ static void
_simple_free(FilterXExpr *s)
{
FilterXSimpleFunction *self = (FilterXSimpleFunction *) s;
filterx_function_args_free(self->args);
g_ptr_array_free(self->args, TRUE);
filterx_function_free_method(&self->super);
}

static inline gboolean
_simple_process_args(FilterXSimpleFunction *self, FilterXFunctionArgs *args, GError **error)
{
guint64 len = filterx_function_args_len(args);

for (guint64 i = 0; i < len; i++)
{
FilterXExpr *expr = filterx_function_args_get_expr(args, i);
if (!expr)
{
g_set_error(error, FILTERX_FUNCTION_ERROR, FILTERX_FUNCTION_ERROR_UNEXPECTED_ARGS, "can't get argument");
return FALSE;
}

g_ptr_array_add(self->args, expr);
}

return TRUE;
}

FilterXExpr *
filterx_simple_function_new(const gchar *function_name, FilterXFunctionArgs *args,
FilterXSimpleFunctionProto function_proto)
FilterXSimpleFunctionProto function_proto, GError **error)
{
FilterXSimpleFunction *self = g_new0(FilterXSimpleFunction, 1);

filterx_function_init_instance(&self->super, function_name);
self->super.super.eval = _simple_eval;
self->super.super.free_fn = _simple_free;
self->args = args;
self->function_proto = function_proto;

self->args = g_ptr_array_new_full(filterx_function_args_len(args), (GDestroyNotify) filterx_expr_unref);
if (!_simple_process_args(self, args, error) ||
!filterx_function_args_check(args, error))
goto error;

filterx_function_args_free(args);
return &self->super.super;

error:
filterx_function_args_free(args);
filterx_expr_unref(&self->super.super);
return NULL;
}

void
Expand Down Expand Up @@ -178,12 +229,6 @@ filterx_generator_function_init_instance(FilterXGeneratorFunction *s, const gcha
s->super.super.free_fn = _generator_function_free;
}

struct _FilterXFunctionArgs
{
GPtrArray *positional_args;
GHashTable *named_args;
};

/* Takes reference of value */
FilterXFunctionArg *
filterx_function_arg_new(const gchar *name, FilterXExpr *value)
Expand Down Expand Up @@ -256,24 +301,7 @@ filterx_function_args_empty(FilterXFunctionArgs *self)
FilterXExpr *
filterx_function_args_get_expr(FilterXFunctionArgs *self, guint64 index)
{
if (self->positional_args->len <= index)
return NULL;

FilterXFunctionArg *arg = g_ptr_array_index(self->positional_args, index);
arg->retrieved = TRUE;
return filterx_expr_ref((FilterXExpr *) arg->value);
}

FilterXObject *
filterx_function_args_get_object(FilterXFunctionArgs *self, guint64 index)
{
FilterXExpr *expr = filterx_function_args_get_expr(self, index);
if (!expr)
return NULL;

FilterXObject *obj = filterx_expr_eval(expr);
filterx_expr_unref(expr);
return obj;
return filterx_expr_ref(_args_get_expr(self, index));
}

static const gchar *
Expand Down Expand Up @@ -305,95 +333,77 @@ _get_literal_string_from_expr(FilterXExpr *expr, gsize *len)
const gchar *
filterx_function_args_get_literal_string(FilterXFunctionArgs *self, guint64 index, gsize *len)
{
FilterXExpr *expr = filterx_function_args_get_expr(self, index);
FilterXExpr *expr = _args_get_expr(self, index);
if (!expr)
return NULL;

const gchar *str = _get_literal_string_from_expr(expr, len);

filterx_expr_unref(expr);
return str;
return _get_literal_string_from_expr(expr, len);
}

gboolean
filterx_function_args_is_literal_null(FilterXFunctionArgs *self, guint64 index)
{
gboolean is_literal_null = FALSE;

FilterXExpr *expr = filterx_function_args_get_expr(self, index);
FilterXExpr *expr = _args_get_expr(self, index);
if (!expr)
goto error;
return FALSE;

if (!filterx_expr_is_literal(expr))
goto error;
return FALSE;

FilterXObject *obj = filterx_expr_eval(expr);
if (!obj)
goto error;
return FALSE;

is_literal_null = filterx_object_is_type(obj, &FILTERX_TYPE_NAME(null));
gboolean is_literal_null = filterx_object_is_type(obj, &FILTERX_TYPE_NAME(null));
filterx_object_unref(obj);

error:
filterx_expr_unref(expr);
return is_literal_null;
}

FilterXExpr *
filterx_function_args_get_named_expr(FilterXFunctionArgs *self, const gchar *name)
_args_get_named_expr(FilterXFunctionArgs *self, const gchar *name)
{
/* 'retrieved' is not thread-safe */
main_loop_assert_main_thread();

FilterXFunctionArg *arg = g_hash_table_lookup(self->named_args, name);
if (!arg)
return NULL;
arg->retrieved = TRUE;
return filterx_expr_ref(arg->value);
return arg->value;
}

FilterXObject *
filterx_function_args_get_named_object(FilterXFunctionArgs *self, const gchar *name, gboolean *exists)
FilterXExpr *
filterx_function_args_get_named_expr(FilterXFunctionArgs *self, const gchar *name)
{
FilterXExpr *expr = filterx_function_args_get_named_expr(self, name);
*exists = !!expr;
if (!expr)
return NULL;

FilterXObject *obj = filterx_expr_eval(expr);
filterx_expr_unref(expr);
return obj;
return filterx_expr_ref(_args_get_named_expr(self, name));
}

FilterXObject *
filterx_function_args_get_named_literal_object(FilterXFunctionArgs *self, const gchar *name, gboolean *exists)
{
FilterXObject *obj = NULL;
FilterXExpr *expr = filterx_function_args_get_named_expr(self, name);
FilterXExpr *expr = _args_get_named_expr(self, name);
*exists = !!expr;
if (!expr)
return NULL;

if (!filterx_expr_is_literal(expr))
goto error;
return NULL;

obj = filterx_expr_eval(expr);
error:
filterx_expr_unref(expr);
return obj;
return filterx_expr_eval(expr);
}


const gchar *
filterx_function_args_get_named_literal_string(FilterXFunctionArgs *self, const gchar *name, gsize *len,
gboolean *exists)
{
FilterXExpr *expr = filterx_function_args_get_named_expr(self, name);
FilterXExpr *expr = _args_get_named_expr(self, name);
*exists = !!expr;
if (!expr)
return NULL;

const gchar *str = _get_literal_string_from_expr(expr, len);

filterx_expr_unref(expr);
return str;
return _get_literal_string_from_expr(expr, len);
}

gboolean
Expand Down Expand Up @@ -455,7 +465,7 @@ filterx_function_args_get_named_literal_generic_number(FilterXFunctionArgs *self
GenericNumber value;
gn_set_nan(&value);

FilterXExpr *expr = filterx_function_args_get_named_expr(self, name);
FilterXExpr *expr = _args_get_named_expr(self, name);
*exists = !!expr;
if (!expr)
return value;
Expand All @@ -480,7 +490,6 @@ filterx_function_args_get_named_literal_generic_number(FilterXFunctionArgs *self
*error = TRUE;
success:
filterx_object_unref(obj);
filterx_expr_unref(expr);
return value;
}

Expand Down Expand Up @@ -531,13 +540,13 @@ filterx_function_args_free(FilterXFunctionArgs *self)


static FilterXExpr *
_lookup_simple_function(GlobalConfig *cfg, const gchar *function_name, FilterXFunctionArgs *args)
_lookup_simple_function(GlobalConfig *cfg, const gchar *function_name, FilterXFunctionArgs *args, GError **error)
{
// Checking filterx builtin functions first
FilterXSimpleFunctionProto f = filterx_builtin_simple_function_lookup(function_name);
if (f != NULL)
{
return filterx_simple_function_new(function_name, args, f);
return filterx_simple_function_new(function_name, args, f, error);
}

// fallback to plugin lookup
Expand All @@ -550,7 +559,7 @@ _lookup_simple_function(GlobalConfig *cfg, const gchar *function_name, FilterXFu
if (f == NULL)
return NULL;

return filterx_simple_function_new(function_name, args, f);
return filterx_simple_function_new(function_name, args, f, error);
}

static FilterXExpr *
Expand Down Expand Up @@ -582,7 +591,7 @@ filterx_function_lookup(GlobalConfig *cfg, const gchar *function_name, GList *ar
if (!args)
return NULL;

FilterXExpr *expr = _lookup_simple_function(cfg, function_name, args);
FilterXExpr *expr = _lookup_simple_function(cfg, function_name, args, error);
if (expr)
return expr;

Expand Down
2 changes: 0 additions & 2 deletions lib/filterx/expr-function.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,9 @@ FilterXFunctionArgs *filterx_function_args_new(GList *args, GError **error);
guint64 filterx_function_args_len(FilterXFunctionArgs *self);
gboolean filterx_function_args_empty(FilterXFunctionArgs *self);
FilterXExpr *filterx_function_args_get_expr(FilterXFunctionArgs *self, guint64 index);
FilterXObject *filterx_function_args_get_object(FilterXFunctionArgs *self, guint64 index);
const gchar *filterx_function_args_get_literal_string(FilterXFunctionArgs *self, guint64 index, gsize *len);
gboolean filterx_function_args_is_literal_null(FilterXFunctionArgs *self, guint64 index);
FilterXExpr *filterx_function_args_get_named_expr(FilterXFunctionArgs *self, const gchar *name);
FilterXObject *filterx_function_args_get_named_object(FilterXFunctionArgs *self, const gchar *name, gboolean *exists);
FilterXObject *filterx_function_args_get_named_literal_object(FilterXFunctionArgs *self, const gchar *name,
gboolean *exists);
const gchar *filterx_function_args_get_named_literal_string(FilterXFunctionArgs *self, const gchar *name,
Expand Down
Loading
Loading