Epoll doesn't remove closed descriptors/files from the interest list

[stevenengler]

When a descriptor/file that an epoll object is watching closes, the epoll object doesn't remove it from its watching table. If the descriptor isn't removed using EPOLL_CTL_DEL, then the epoll object will never unref that descriptor/file and it will remain in the watching table until the epoll descriptor itself is closed.

Will closing a file descriptor cause it to be removed from all epoll interest lists?

Yes, but be aware of the following point. [...] A file descriptor is removed from an interest list only after all the file descriptors referring to the underlying open file description have been closed.

I think when _epoll_descriptorStatusChanged() runs, it should check if the descriptor/file has been closed (the EpollWatch flag EWF_CLOSED is set) and if so remove it from the watching table.

[robgjansen]

There is a complication here because there are some sockets can still be read after they are closed, for example, if they still have data to read sitting in the buffer.

[robgjansen]

I just found that if an fd is first closed, and then epoll_ctl(EPOLL_CTL_DEL) is called on that fd, Shadow thinks that the fd is invalid and will not tell epoll to remove the fd from the watching table.

[stevenengler]

I just found that if an fd is first closed, and then epoll_ctl(EPOLL_CTL_DEL) is called on that fd, Shadow thinks that the fd is invalid and will not tell epoll to remove the fd from the watching table.

I think that's the standard Linux behaviour. You need to EPOLL_CTL_DEL before closing the fd.

https://idea.popcount.org/2017-03-20-epoll-is-fundamentally-broken-22/

[robgjansen]

Cool. In my current debug session, it looks like Tor (or one of its dependencies) is doing things in the incorrect order (close() and then EPOLL_CTL_DEL).

[stevenengler]

Cool. In my current debug session, it looks like Tor (or one of its dependencies) is doing things in the incorrect order (close() and then EPOLL_CTL_DEL).

That might be this:

https://github.com/libevent/libevent/blob/852af060af7c7e439f0db97829ec74bedeba64d5/epoll.c#L375-L386

[robgjansen]

Looks like we can just add an if statement after the child is removed from the ready table here, to then also check if the child status is CLOSED to then also remove it from the waiting table too.

https://github.com/shadow/shadow/blob/91175680e76cf9e4c263ef1db2db06232c8c1c84/src/main/host/descriptor/epoll.c#L725

We probably also want to create a test case.

[stevenengler]

@robgjansen There's a comment in Shadow that refers to this issue:

https://github.com/shadow/shadow/blob/e81eab700f6dea596b8a65a54060f3a420706b8b/src/main/host/syscall/epoll.c#L108-L121

Do you remember why this was added (in #1169)? This issue is closed now, so maybe we can remove this block? Why is this only for legacy descriptors, and what's special about EPOLL_CTL_DEL? I don't think it should be possible to have a file handle (an fd) to a closed file object?

[robgjansen]

It looks like the && op != EPOLL_CTL_DEL part was added because it was causing errors in libevent/tor, but that should no longer be a problem anymore now that #1101 is fixed. So I think we should be able to remove the && op != EPOLL_CTL_DEL part.

I'm not sure if we can delete the entire if {} block. Don't we still want to call _syscallhandler_validateLegacyFile on the child fd? Or do you think that block is dead code?

[stevenengler]

I'm not sure if we can delete the entire if {} block. Don't we still want to call _syscallhandler_validateLegacyFile on the child fd? Or do you think that block is dead code?

I thought it would be dead code, but it looks like TCP sockets set the CLOSED state themselves even if there are still file handles. So I don't think we can get rid of this block until we rewrite the TCP code.