Repository navigation
uloop improvements - #478
Merged
Merged
Conversation
nbd168
commented
Oct 9, 2026
nbd168
commented
Contributor
- drop references on idle timers, fds, etc. to allow cleanup
- expose more uloop flags
- fix edge cases on closing fds with open uloop handles.
nbd168
force-pushed
the
uloop
branch
2 times, most recently
from
October 9, 2026 14:40
da610de to
3b004b1
Compare
Collaborator
|
Most of it looks sane, but I tripped over two commits:
|
Some behaviour that a testcase checks exists only on one platform, for example epoll semantics or /proc/self. There is no way to mark such a testcase, and the runner aborts on an unknown section. Add a "-- Platform --" section that lists the platforms, as reported by "uname -s", on which the testcase runs. On any other platform the testcase is skipped and counted in the summary line of its file. Signed-off-by: Felix Fietkau <nbd@nbd.name>
A uloop task runs in a child that fork() created without exec() and that ends with _exit() while it still holds the heap of the parent. Valgrind follows such a child and reports the inherited allocations as possibly lost, which fails every testcase that starts a task. Leave forked children unchecked. The parent process, which runs the code under test, is still checked in full. Signed-off-by: Felix Fietkau <nbd@nbd.name>
Some testcases depend on features that a module only provides when its build dependencies have them, for example uloop.interval() and uloop.signal() with an older libubox. Such a testcase fails, although there is nothing to test. Treat exit code 77 as a skipped testcase, as automake does, unless the testcase expects that exit code. Its output is not compared, and it is counted in the summary line of its file. Signed-off-by: Felix Fietkau <nbd@nbd.name>
On EPOLLERR or EPOLLHUP, libubox removes a watched descriptor from the event loop unless the handle was registered with ULOOP_ERROR_CB. Some of these errors are transient: a connected UDP socket that receives an ICMP port unreachable reports EPOLLERR once and stays usable. A script cannot request the flag, so such a handle stops receiving events after the first error. Export the constant. The callback already receives the error state as its third argument. Signed-off-by: Felix Fietkau <nbd@nbd.name>
Every uloop instance keeps a reference to itself so that it survives without a script reference while it is registered. For timers and handles, only cancel() and delete() dropped that reference. A one-shot timer that fired thus stayed allocated forever, together with its callback closure and everything the closure captured, unless the script cancelled the expired timer. The same applied to a handle whose descriptor uloop removed after an error or hangup. Track the self reference separately from the instance pointer and hold it only while the event source is live: - a timer holds it while it is armed, so an expired timer that was not rearmed from its callback is freed as soon as the script drops it; a retained timer can still be rearmed with set() - a handle drops it once uloop removed its descriptor, which cannot be registered again - processes and tasks already released themselves on completion; intervals and signal handlers stay registered until cancelled cancel() and delete() behave as before: they detach the instance, drop its callbacks and release it. The free callbacks now only detach from uloop, as the resource values are already released when they run. The timer, handle and process callbacks hold a temporary reference while they use the instance after invoking the script callback, which may drop the last reference by calling cancel() or delete(). A handle now takes its self reference only after uloop_fd_add() succeeded. The error path therefore drops the one reference that exists, in place of the teardown that commit e6e0300 ("uloop: release the handle when uloop_fd_add() fails") added for the self reference. Signed-off-by: Felix Fietkau <nbd@nbd.name>
The documentation of timer.set() treats a missing timeout as "not armed", and its example disables a timer with set(). set() passed -1 to uloop_timeout_set() instead, which schedules the timer in the past, so the timer fired on the next loop iteration. Cancel the pending timeout when no timeout or null is given, and keep the callback, so that the timer can be rearmed later. A disarmed timer is no longer held by the event loop and is freed once the script drops it. A negative timeout is still passed to uloop_timeout_set() and expires the timer on the next iteration, as in libubox. Document both cases. Signed-off-by: Felix Fietkau <nbd@nbd.name>
interval.set() passes -1 to uloop_interval_set() when no value is given, as it does for any negative value. That function takes an unsigned interval, so the interval is rearmed to fire about every 49.7 days instead of being disabled, as the documentation of set() states. Cancel the interval when no value or null is given, and keep the callback, so that it can be rearmed later, as timer.set() does. Reject a zero or negative interval with EINVAL, as libubox has no meaning for either: with the epoll backend, a zero interval disarms the timerfd but keeps it registered, and remaining() then reports 0. For the same reason, interval() now leaves an interval of 0 unarmed, as it does for a negative value. Fixes: 0352a33 ("uloop: support new interval and signal APIs") Signed-off-by: Felix Fietkau <nbd@nbd.name>
Commit af8a383 ("uloop: close the task output pipe even without an input callback") closes the output pipe of a task and removes its process from uloop, whether or not the task has an input callback. Add a test that a finished task leaves no descriptor open, with and without an input callback. Signed-off-by: Felix Fietkau <nbd@nbd.name>
The task output callback keeps using the task after it invoked the script's output or input callback. If that callback runs a nested uloop.run(), the task can finish and be released meanwhile, and the only remaining reference is the one the VM holds for the call. Once the callback returns, the task is freed and the loop reads the next message from freed memory. Hold a reference for the duration of the output callback, as the timer, handle and process callbacks do. Signed-off-by: Felix Fietkau <nbd@nbd.name>
When the output or input callback of a task raised an exception, the task output callback returned before it checked whether the task had finished. If the exception happened in the last invocation, after uloop removed the output pipe and the process exited, the task was never cleaned up: it stayed registered with its callbacks, and its output pipe stayed open. Stop reading messages on an exception, but still run the cleanup check. Signed-off-by: Felix Fietkau <nbd@nbd.name>
uloop_fd_add() returns success without registering the descriptor when the flags contain no IO event, for example ULOOP_ERROR_CB or ULOOP_EDGE_TRIGGER alone. handle() then returned an instance that never fired and that the event loop kept alive until delete(). libubox cannot watch a descriptor for errors only, so fail with EINVAL when the descriptor did not get registered. Signed-off-by: Felix Fietkau <nbd@nbd.name>
A handle refers to its descriptor by number. If the script closes the file or socket before it deletes the handle, and no other descriptor refers to that file, the kernel drops the file from epoll. The number can then be reused by a new descriptor that another handle watches. delete(), or freeing the handle, then passed the number to EPOLL_CTL_DEL and silently unregistered the other handle. Record the device and inode of the descriptor when the handle is created. When they no longer match on delete, only clear the uloop state and leave epoll alone. Descriptors on the shared anonymous inode, such as eventfd or timerfd, cannot be told apart this way. A dup of the descriptor would avoid the problem, but it would keep the file open and firing after the script closed it. The event loop still cannot notice a closed descriptor by itself, so document that delete() must come first. Signed-off-by: Felix Fietkau <nbd@nbd.name>
Contributor
Author
|
I thought about this some more. Turns out the level trigger emulation is only necessary to prevent busy spinning on broken scripts that close and reopen a fd before deleting the handle. The memory safety issues are fixed by other commits that don't carry this overhead. I'll drop this emulation. The second point makes sense. I'll push an update on that. |
If the script closes the descriptor of a handle while another descriptor keeps the file open, for example a dup or a child process that inherited it, the file stays in epoll under the closed number, with the handle as its event data. Nothing can remove that entry by number any more. Once the handle was deleted and freed, or unpinned after uloop dropped it on an error, the next event on the file made uloop call into freed memory. When the descriptor no longer refers to the watched file, turn the handle into a dead one: clear the uloop state without touching epoll, point the uloop callback at a function that ignores events, release the script callbacks and keep the instance pinned for good. uloop removes a descriptor on error by number, before the handle can check it. Always register with ULOOP_ERROR_CB and perform that removal in the handle callback instead, where the check applies. A dead handle stays allocated for good. This also happens when no other descriptor keeps the file open and epoll no longer refers to the handle, where the instance was freed before: without the epoll descriptor of libubox, the module cannot tell the two cases apart, and a leaked instance replaces a possible use-after-free. A level-triggered event on the file of a dead handle keeps the event loop busy until the other holder closes the file. Both only happen when a script closes a descriptor before deleting its handle. Signed-off-by: Felix Fietkau <nbd@nbd.name>
A script may close the descriptor of a handle without delete() and open the same path again, which yields the same number, device and inode. The fstat() check then passes while another descriptor keeps the old file in epoll, and deleting the old handle removes the entry by number: - if another handle watches the new file, EPOLL_CTL_DEL removes that handle's entry, and the other handle receives no more events; - otherwise EPOLL_CTL_DEL fails with ENOENT, and the old handle is treated as removed, unpinned and freed, while epoll still reports the old file to it. A failed EPOLL_CTL_DEL proves that the number no longer refers to the watched file, so disable the handle then instead of releasing it. Also track the live handles of the process, and disable older handles on a number when a new handle registers it, as the new registration proves that they lost their file. A disabled handle stays allocated for good, as a dead handle does. uloop_done() closes the epoll instance, after which every removal fails. Unregister the live handles in uloop.done() before that, and release them like handles whose descriptor uloop removed, so that a later delete() does not take them for handles that lost their file. Descriptors watched outside of this module, for example by the application that embeds ucode, are not tracked, so a reopened path still affects them; document this. Sockets, such as those of ubus, rtnl or nl80211, are not affected, since every socket has its own inode. Signed-off-by: Felix Fietkau <nbd@nbd.name>
libubox maps ULOOP_PRIORITY to EPOLLPRI and reports it to the callback,
but the module does not export the constant. A script cannot watch for
priority data, for example TCP urgent data.
Export the constant when libubox defines it, since it was only added in
libubox commit 895f92164b66 ("uloop: add ULOOP_PRIORITY support for
EPOLLPRI events"). The kqueue backend ignores the flag.
Signed-off-by: Felix Fietkau <nbd@nbd.name>
Since commit 1e6b565 ("uloop: read task messages without losing them to EAGAIN"), the parent's end of a task output pipe is blocking. uc_uloop_task_output_cb() reads messages in a loop until a read fails, so after the last available message it waits in read() until the task sends the next one or exits. The event loop stalls for as long as the task works between two messages, and no timer, handle or other task is served meanwhile. Watch the pipe non-blocking again, and keep the state of a partly received message in the task: the bytes of the length header received so far, the number of payload bytes left, and the JSON tokener, which parses the payload as it arrives. A read never crosses the end of a message, so a callback that runs a nested uloop.run() on the same task still sees the messages in order. pipe.receive() in the child shares the tokener handling. An exception in an output or input callback no longer stops the reads, so the messages that follow it are still delivered when the task has already finished. A corrupt length header closes the pipe. Fixes: 1e6b565 ("uloop: read task messages without losing them to EAGAIN") Signed-off-by: Felix Fietkau <nbd@nbd.name>
uc_uloop_task() creates the input and output pipes before it calls fork(), but returns without closing them when fork() fails. Every failed task() call leaks four descriptors, for example while the process limit makes fork() fail with EAGAIN. Close the pipes on that path as on a pipe() failure. Fixes: 28ee7e1 ("uloop: add support for tasks") Signed-off-by: Felix Fietkau <nbd@nbd.name>
Collaborator
|
Merged, thanks! |
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
2 participants
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.