-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to close thread dispatcher #326
base: master
Are you sure you want to change the base?
Conversation
proc closeThreadDispatcher*() {.raises: [Defect, CatchableError].} = | ||
if gDisp.isNil: return | ||
let prevDisp = gDisp | ||
while gDisp.callbacks.len > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely not enough, number of callbacks could zero right after poll()
call but it doesn't mean that there is no pending socket events, or pending timers. And even in such case you should check if this callback is actually SentinelCallback
. Call to closeThreadDispatcher
could be made while main poll()
loop processing callbacks, so its possible to get into situation when callbacks <= 1
, but even in such case its possible to get into situation when there no SentinelCallback
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll check that this is the sentinel, and that it's not called from inside poll
Closing IOCP port or EPOLL/KQUEUE file descriptor do not closes all the files/sockets/pipes which was registered with it. So with this PR the only resource you are cleaning is IOCP port or EPOLL/KQUEUE file descriptor - nothing more. Your PR do not even check if there some descriptors registered in IOCP/EPOLL/KQUEUE. |
I would argue that the sockets the user opened are to be closed by the user We can imagine that a user does something where he opens a socket on one thread, and then switch its processing to another thread, so we shouldn't close it if he stops the first thread |
Of course, but i think the main idea of calling |
When a thread finishes, there is currently no way to release the resources of the dispatcher. Which on linux, include a FD & relatively big buffer for
epoll
We could use
onThreadDestruction
to call "close" automatically, but that raises questions in what to do with pending futures