Clean up main loop child and timer code, and deprecate their public API - #4167
Open
nrwahl2 wants to merge 70 commits into
Open
Clean up main loop child and timer code, and deprecate their public API#4167nrwahl2 wants to merge 70 commits into
nrwahl2 wants to merge 70 commits into
Conversation
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
mainloop_child_timeout() is public API, so we have to convert to gboolean (int) there. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
...and mainloop_child_add(). * Add Doxygen. * Improve variable names and comments. * Drop redundant assignments. All fields are zero-initialized. * Add TODO comment for flags argument. The comment mentions bool as a possibility, because currently there's only one enum value, so we may not need a flag group once this is internal. * Use bool instead of gboolean. * Be more assertive about passing a positive value for the child PID. Some functions like child_kill_helper() will definitely not work correctly for a nonpositive PID. * Fix a minor overflow bug. Note that pcmk__create_timer() takes an unsigned int for its interval_ms argument. So we want to avoid passing a negative int to pcmk__create_timer() here, since the negative value would be cast to a very large positive unsigned value. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
* Add Doxygen. * Improve log messages and comments. * Rename to install_sigchld_handler(). * Rename argument to user_data to align with GSourceFunc definition. * Return G_SOURCE_REMOVE instead of FALSE. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The name "timeout" made it sounds as if it held the timeout duration in my opinion. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There's only one enum value, so replace the flags field with a boolean. Also invert its meaning, since I think the inverted semantics are more clear. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
So that we can use g_list_find_custom(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We've already got the link (as match). No need to have g_list_remove() iterate over the list again to find it. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
* Add Doxygen. * Rename argument to user_data. * Return G_SOURCE_REMOVE instead of FALSE. * Improve log messages and comments. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Previously, if we failed to kill a child process that timed out, we would log a warning if it hadn't terminated after five more seconds. This doesn't seem useful to me. If kill() failed, we already log an error for that. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Previously, ESRCH (child already terminated) was the only case where we weren't setting child->timed_out to true. However, if we reach child_timeout_callback() and then kill() returns ESRCH, that means the child process terminated after the timeout expired. If a child terminates before the timeout expires, the SIGCHLD handler (child_death_dispatch()) destroys the timeout source and frees the mainloop_child_t. Since the timeout source is destroyed in that case, child_timeout_callback() won't get called for that child. The SIGCHLD handler is implemented as a main loop trigger with priority (G_PRIORITY_HIGH - 1) -- that is, -100 - 1 == -101. The timeout source has priority G_PRIORITY_DEFAULT -- that is, 0. A more negative value indicates a higher priority. So if a child terminates before the timer expires, then the trigger for the SIGCHLD handler should be dispatched in a main loop iteration **before** the iteration that dispatches the timeout source. (On each main loop iteration, GLib finds the highest- priority source that's ready. It then dispatches all ready sources that have that same priority. Ready sources of a lower priority must wait until a later iteration to be dispatched.) Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
* Add Doxygen. * Rename to kill_child_pid(). * Simplify debug logging. It's known that a negative PID kills the process group. * Store the PID or its negation in a variable for convenience. * Log the child's description. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
* Add Doxygen. * Rename to free_terminated_children(). * Use a while loop, which feels more natural when the current element might get removed. Frustratingly, there is no g_list_foreach_remove() corresponding to g_hash_table_foreach_remove(). * Use g_list_delete_link() in place of g_list_remove_link() and g_list_free(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The only callers pass either 0 or WNOHANG for the flags argument. Commit c2dbdfb, which added this case and the comment about WUNTRACED and WCONTINUED, acknowledges that "[t]his has no effect in practice since no callers use the flags necessary to reach that condition." Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The only values we passed for the flags argument were 0 and WNOHANG. Also use pid_t for rc, since that's what waitpid() returns. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
As noted in the now-deleted comment, we were previously capturing too many cases with the same test. This could create misleading log messages. This commit is not intended to change any behavior. Variables and return values are intended to keep their same values. This is intended to change only the log messages. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
WCOREDUMP() is supposed to be called only if WIFSIGNALED() returns true. Previously, we called it only if WIFSIGNALED() returned false. This also meant that if a process was terminated by a signal, we would never mark it as having dumped core. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The (rc != child->pid) case means that child->pid is nonpositive and a process in the group with ID -child->pid has changed state. It doesn't make sense to set signo to SIGCHLD or to set exitcode to 1. SIGCHLD did not cause process with ID child->pid to terminate, nor do we know from this check that the child exited with nonzero exit status. Nothing internal creates a child with nonpositive PID, so in practice this change has no effect. It would only matter for a public API caller that creates a child with nonpositive PID and an exit callback. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Commits 348bb51 and c2dbdfb helped me to feel more confident about the intent behind the return code. I really hate that we have allowed nonpositive PIDs via the public API, as it makes the child tracking behavior messy, poorly specified, and/or probably incorrect in several places. It seems like the simplest approach in most places is to preserve that behavior until we get rid of the public API. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
...to callback. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Rename t to timer, add whitespace, unindent a bit, etc. These changes are simple enough that I elected to do them all in one commit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
If timer->cb were NULL, mainloop_timer_cb() would do nothing except set timer->id to 0 and remove the timeout source. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The callers that passed false or FALSE for the repeat argument also passed a callback that always returned G_SOURCE_REMOVE (or equivalently, FALSE). mainloop_timer_t:repeat acts as an override switch for the callback's return value. If timer->repeat is true, mainloop_timer_cb() returns the return value from calling timer->cb. If timer->repeat is false, mainloop_timer_cb() acts as if timer->cb returned G_SOURCE_REMOVE. So for our internal callers of mainloop_timer_add(), the callbacks themselves give enough information via their return codes. We don't need to override them. Also replace TRUE with true in some other calls for consistency. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It's always set to crm_timer_popped() and it just made the pcmk__create_timer() call a little bit harder to trace. Move crm_timer_popped() just so that we can use it in the pcmk__create_timer() call. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
We call pcmk__assert_alloc() to allocate these timers. That sets all the fields to zero. So drop assignments that set fields to 0 or FALSE. Keep the assignment of I_NULL to wait_timer->fsa_input, just because it's not as obvious that that value is equivalent to zero. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
It always returned true previously. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
The override from I_FINALIZED to I_ELECTION has been present for over 20 years (since commit b6f3e49). There's no reason to do the initial set to I_FINALIZED. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Name the one in attrd explicitly, and remove whitespace from the one in pacemakerd. This will allow us to assert name is not NULL in an internal constructor for a main loop timer. Also assert that name is not NULL in attrd_create_attribute(). This is currently enforced by attrd_peer_update_one(). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace mainloop_timer_add() internally. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace mainloop_timer_running() internally. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace mainloop_timer_del() internally. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
There are only two callers, and right now I think I'd rather they be explicit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
mainloop_timer_cb() sets timer->id to 0 before calling timer->cb. So pcmk__main_loop_timer_running() would return false when called as part of get_agent_metadata_cb(), which is timer->cb in this case. Thus mainloop_timer_start() would never get called. The code that we're removing here is dead, but the period increase seems broken as noted in the FIXME comment. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace mainloop_timer_start() internally. All internal main loop timers have nonzero period and non-NULL callback. Add a temporary forward declaration for mainloop_timer_cb(). It will go away in an upcoming commit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace mainloop_timer_stop() internally. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace mainloop_timer_cb() in pcmk__main_loop_timer_start(). The names are confusing, but the plan is for the old one to go away in the future. Note that we can ignore timer->repeat, because it's true for all the timers that will get passed to this callback. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To me this seems clearer and more consistent with other code (aside from mainloop_timer_t, which will change next). Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To me this seems clearer and more consistent with other code. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
I want to make it more clear that this is not the ID of the mainloop_timer_t object. If the timer is running, this field is set to the ID of the GSource that we added with pcmk__create_timer(). Otherwise, it's set to 0 to indicate that there is no such GSource. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
To replace mainloop_timer_t internally. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
Deprecate the mainloop_timer_t type itself and all functions that accept or manipulate mainloop_timer_t objects. External programs should not use Pacemaker for general-purpose asynchronous programming. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@clumens This is super low priority right now. We both have other outstanding work at time of writing, including other PRs to review and to address review of.
Also, it would be reasonable to split this up between the child-related commits and the timer-related commits, unless you feel like reviewing them all at once.
I decided to file this so that the work is out there if anything happens to me (please no), and because these parts are ready for review whenever.