Skip to content

Commit

Permalink
Rework 'wexecvp' replacing legacy 'select' in favor of 'poll' #4001
Browse files Browse the repository at this point in the history
This commit addresses several issues with 'wexecvp':
  - Fix invalid double call to 'close' in case fd was higher than
    'FD_SETSIZE'. This could lead to the invalidation of an unrelated
    fd resulting in asserts, as in issue #4001 and other instabilities.
  - Fix previous limitations of the legacy 'select 'impl that rendered
    the RESTAPI unusable when ProxySQL had more then 'FD_SETSIZE' fds
    opened.
  - Other minor improvements in function logic and interface.
  • Loading branch information
JavierJF committed Dec 9, 2022
1 parent 6373e96 commit f8158dc
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 136 deletions.
19 changes: 9 additions & 10 deletions include/proxysql_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -128,27 +128,27 @@ cfmt_t cstr_format(char (&out_buf)[N], const char* fmt, ...) {
/**
* @brief Simple struct that holds the 'timeout options' for 'wexecvp'.
*/
struct to_opts {
struct to_opts_t {
/**
* @brief Timeout for the script execution to be completed, in case of being
* exceeded, the script will be terminated.
*/
unsigned int timeout_us;
/**
* @brief Timeout used for 'select()' non blocking calls.
* @brief Timeout used for 'poll()' non blocking calls.
*/
suseconds_t select_to_us;
suseconds_t poll_to_us;
/**
* @brief The duration of the sleeps between the checks being performed
* on the child process waiting it to exit after being signaled to terminate,
* before issuing 'SIGKILL'.
*/
unsigned int it_delay_us;
unsigned int waitpid_delay_us;
/**
* @brief The timeout to be waited on the child process after being signaled
* with 'SIGTERM' before being forcely terminated by 'SIGKILL'.
*/
unsigned int sigkill_timeout_us;
unsigned int sigkill_to_us;
};

/**
Expand All @@ -165,17 +165,16 @@ struct to_opts {
* @return 0 In case of success or one of the following error codes:
* - '-1' in case any 'pipe()' creation failed.
* - '-2' in case 'fork()' call failed.
* - '-3' in case 'fcntl()' call failed .
* - '-4' in case of resource exhaustion, file descriptors being used exceeds 'FD_SETSIZE'.
* - '-5' in case 'select()' call failed.
* - '-6' in case 'read()' from pipes failed with a non-expected error.
* - '-3' in case 'fcntl()' call failed.
* - '-4' in case 'poll()' call failed.
* - '-5' in case 'read()' from pipes failed with a non-expected error.
* - 'ETIME' in case the executable has exceeded the timeout supplied in 'opts.timeout_us'.
* In all this cases 'errno' is set to the error reported by the failing 'system call'.
*/
int wexecvp(
const std::string& file,
const std::vector<const char*>& argv,
const to_opts* opts,
const to_opts_t& opts,
std::string& s_stdout,
std::string& s_stderr
);
Expand Down
10 changes: 5 additions & 5 deletions lib/ProxySQL_RESTAPI_Server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,17 +97,17 @@ class sync_resource : public http_resource {
if (nullptr!=result)
return result;

to_opts wexecvp_opts {};
to_opts_t wexecvp_opts {};
wexecvp_opts.timeout_us = (interval_ms * 1000);
wexecvp_opts.select_to_us = 100*1000;
wexecvp_opts.it_delay_us = 500*1000;
wexecvp_opts.sigkill_timeout_us = 3000 * 1000;
wexecvp_opts.poll_to_us = 100*1000;
wexecvp_opts.waitpid_delay_us = 500*1000;
wexecvp_opts.sigkill_to_us = 3000 * 1000;

std::string script_stdout {""};
std::string script_stderr {""};

const std::vector<const char*> args { const_cast<const char*>(params.c_str()), NULL};
int script_err = wexecvp(script.c_str(), args, &wexecvp_opts, script_stdout, script_stderr);
int script_err = wexecvp(script.c_str(), args, wexecvp_opts, script_stdout, script_stderr);
int script_errno = errno;

std::string str_response_err {};
Expand Down
Loading

0 comments on commit f8158dc

Please sign in to comment.