Potential bug in keep_subprocess_alive method: Unexpected behavior when modifying list during iteration - New multiprocess manager

[yaelmi3]

Related to New multiprocess manager update
Version 0.30.1
In the keep_subprocess_alive method, there is a potential and unpleasant bug related to modifying a list while iterating over it. The method iterates over the self.processes list and potentially removes elements from it. If a process is not alive, it gets removed from the list. However, because the list is being modified during iteration, it's possible that the next process in the list gets skipped over in the iteration, leading to incorrect behavior.

This issue has been observed in a production environment where some worker processes became unresponsive. Instead of terminating these unresponsive workers, the parent process continued to spawn new workers. This resulted in an accumulation of unresponsive worker processes, as they were not being correctly removed from the self.processes list.

To reproduce the issue, I've added a return False to the process.is_alive method. Here's the output when running the server with two workers. I've added some print to emphasize the issue (process 75777 died, but we delete process 75782, for example):

> uvicorn cronenberg.server:app --host 0.0.0.0 --port 5000 --workers 2
INFO:     Uvicorn running on http://0.0.0.0:5000 (Press CTRL+C to quit)
INFO:     Started parent process [75772]
Processes before keep_subprocess_alive: [75776, 75777]
INFO:     Waiting for child process [75776]
INFO:     Child process [75776] died
deleting process self.processes[idx].pid=75776 idx=0
Processes after deletion: [75777]
Processes after addition: [75777, 75782]
Processes before keep_subprocess_alive: [75777, 75782]
INFO:     Waiting for child process [75777]
INFO:     Child process [75777] died
deleting process self.processes[idx].pid=75782 idx=1
Processes after deletion: [75777]
Processes after addition: [75777, 75783]
Processes before keep_subprocess_alive: [75777, 75783]
INFO:     Waiting for child process [75777]
INFO:     Child process [75777] died
deleting process self.processes[idx].pid=75777 idx=0
Processes after deletion: [75783]
Processes after addition: [75783, 75785]
Processes before keep_subprocess_alive: [75783, 75785]
INFO:     Waiting for child process [75783]
INFO:     Child process [75783] died
deleting process self.processes[idx].pid=75785 idx=1
Processes after deletion: [75783]
Processes after addition: [75783, 75787]

Possible solution to this bug:

def keep_subprocess_alive(self) -> None:
    if self.should_exit.is_set():
        return  # parent process is exiting, no need to keep subprocess alive

    alive_processes = []
    for process in self.processes:
        if process.is_alive():
            alive_processes.append(process)
            continue

        process.kill()  # process is hung, kill it
        process.join()

        if self.should_exit.is_set():
            return

        logger.info(f"Child process [{process.pid}] died")
        new_process = Process(self.config, self.target, self.sockets)
        new_process.start()
        alive_processes.append(new_process)

    self.processes = alive_processes

A more simplified python example, to emphasise the issue

numbers = [1, 3, 4, 5, 3, 3]
for number in numbers:
     if number == 3:
         numbers.remove(number)

print(numbers)
-> [1, 2, 4, 5, 3]

[maxfischer2781]

For deleting by index, it's sufficient to iterate in reverse. This ensures that any moved elements have already been processed and that newly appended element are not processed.

So instead of

uvicorn/uvicorn/supervisors/multiprocess.py

Line 166 in 44a3071

for idx, process in enumerate(tuple(self.processes)):

it should suffice to do

for idx, process in tuple(enumerate(self.processes))[::-1]:

instead. There are various alternatives to code this ("enumerate in reverse") as per personal preference.

[xunhanliu]

please use hypercorn ! I have stack in this problem 2days! https://blog.csdn.net/gitblog_00116/article/details/141153910

[maxfischer2781]

Since process are deleted and then replaced it should also be sufficient to just replace them. This ensures the remaining elements are kept in place.

So instead of

uvicorn/uvicorn/supervisors/multiprocess.py

Lines 177 to 180 in 44a3071

	del self.processes[idx]
	process = Process(self.config, self.target, self.sockets)
	process.start()
	self.processes.append(process)

it should suffice to do

process = Process(self.config, self.target, self.sockets)
process.start()
self.processes[idx] = process

instead.

[yaelmi3]

@maxfischer2781 : Thank you for the quick response! When can we expect a fix? We really want to update to the newest version in our production environment and enjoy the new features :)
On a side note, while I'm happy with the proposed solution, I tend to favor creating a new list in such scenarios, as it offers several benefits. It preserves the original list, avoids index errors that can occur when modifying a list during iteration, and enhances code readability. Moreover, it aligns with the principles of functional programming, where functions return new outputs without changing their inputs. This approach can make the code easier to test and reason about. However, I understand that the choice between creating a new list and modifying the existing one in-place depends on the specific requirements and constraints of the application.

[Kludex]

PR welcome.

[yaelmi3]

Thanks for the fix PR @Kludex

[xunhanliu]

please use hypercorn ! I have stack in this problem 2days! https://blog.csdn.net/gitblog_00116/article/details/141153910

[xunhanliu]

please use hypercorn ! I have stack in this problem 2days! https://blog.csdn.net/gitblog_00116/article/details/141153910

[Kludex]

Hypercorn is a great web server implementation. I'm a big fan of the creator. 🙏