search

nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.55k stars 1.47k forks source link

selectors API encourages signal file descriptor leak #12354

Open disruptek opened 5 years ago

disruptek commented 5 years ago

I made a selector like this:

type
    Monitor = enum
        Output = "the process has some data for us on stdout"
        Errors = "the process has some data for us on stderr"
        Finished = "the process has finished"

var
    process = startProcess("/some/long/process/with/output", [])
    watcher = newSelector[Monitor]()

watcher.registerProcess(process.processId, Finished)

# ... some things happened ...

watcher.close

This is leaking the file descriptor returned by registerProcess.

one solution

I would like close to unregister any extant event listeners.

another solution

A lesser solution might be to make registerProcess not be {.discardable.}.

Thanks to @zevv for help debugging.

Nim Compiler Version 1.0.99 [Linux: amd64]
Compiled at 2019-10-03
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: c51857f4348823f647110e8a1ede07d76d93b7da
active boot switches: -d:danger
dom96 commented 5 years ago

A lesser solution might be to make registerProcess not be {.discardable.}.

That is the solution, of course now it'll break code... So silly that registerProcess is discardable.

So the next best solution is to make the close for Selector throw an error when it has some file descriptors that haven't been closed. But... that also has the potential to break code...

I think we're left with only one option: deprecate registerProcess and introduce a new proc that isn't discardable.

zevv commented 5 years ago

dom96, what is wrong with making close() cleanup any un-unregistered resources?

dom96 commented 5 years ago

One problem I can think of is: what if you have two selectors and register the same FDs into both?

disruptek commented 5 years ago

Why would that be a problem?

dom96 commented 5 years ago

because closing one of the selectors will close FDs that belong to another selector

disruptek commented 5 years ago

Nah.

dom96 commented 5 years ago

As discussed on IRC, this is a problem and I think we should solve this by deprecating registerProcess and introducing a non-discardable version of it with a new name.

disruptek commented 4 years ago

Appears to be an identical problem with registerTimer...