Always unsubscribe process_monitor fds before closing them (#2529) #2574
Conversation
| @@ -565,15 +579,6 @@ actor ProcessMonitor | |||
| _stdin_writeable = false | |||
| _stdout_readable = false | |||
| _stderr_readable = false | |||
| if _stdin_event isnt AsioEvent.none() then | |||
There was a problem hiding this comment.
why not just move it right before the _close_fd calls in this function?
From my point of view the unsubscription does not belong in the _close_fd function but here inside _close(), unless _close_fd(fd) renamed/repurposed, but i would strongly vote for keeping it here.
There was a problem hiding this comment.
_close_fd is called from several different places besides _close, so _pending_reads, _write_final, _pending_writes all call it if the read/write operation returns EOF, and _done_writing also needs to unsubscribe and close the fd. Putting the unsubscribe here in _close_fd means that the unsubscribe is handled once in exactly the right place, right before close. Moving unsubscribe out of here means we need another _unsubscribe_fd function, or _close_fd could be renamed to _unsubscribe_and_close_fd, but conceptually unsubscribe and close should be considered to be one operation (IMO).
There was a problem hiding this comment.
Makes sense. Yes, this should be one operation.
Could you add some distilled form of your comment above to the docstring, so it is clear why unsubscribing needs to be happening here? For future reference.
There was a problem hiding this comment.
Please take a look at 05a2ea9 and let me know what you think.
There was a problem hiding this comment.
I think this is a great docstring!
|
Great stuff! I can verify that this is also the way asio unsubscription and file descriptor closing is done for UDP and TCP sockets in the |
|
Once I realised what the problem was I checked the other modules that do asio subscribe/unsubscribe like net, but they were ok from the start. |
jemc
added
the
changelog - fixed
…2529) (ponylang#2574) * Always unsubscribe process_monitor fds before closing them (ponylang#2529) * Update docstring of _close_fd function of ProcessMonitor
This is a 99% fix for #2529; instead of the test program in the issue failing almost immediately with 10 subprocesses, it fails after somewhere in the region of 3k to 50k iterations, depending on whether ponyc is a debug build, has extra debugging printfs inserted, etc. When some extra code is inserted into the epoll asio backend to check the return value of epoll_ctl(..., EPOLL_CTL_DEL,...), call is always failing as the fd is already closed. I don't quite understand why the test program eventually hangs; it almost seems like some events are not being posted to the receiving actor. This needs further testing, so please leave #2529 open.