selectors API encourages signal file descriptor leak

[disruptek]

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]

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]

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

[dom96]

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

[disruptek]

Why would that be a problem?

[dom96]

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

[disruptek]

Nah.

[dom96]

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]

Appears to be an identical problem with registerTimer...