Extend the integration test harness to track FDs #2411
Conversation
Motivation This patch extends the NIO integration test harness to track file descriptors, in particular to search for leaks. This change has been validated on Linux and Darwin, and in both cases correctly diagnoses FD leaks. The goal is to enable us to regression test for things like Modifications - Add support for hooking socket and close calls. - Wire up this support into the test harness. - Extend the test harness to handle the logging. - Add new regression test for apple#2047. Results We can write regression tests for FD leaks.
I'm doing this for speed reasons
|
|
||
| int replacement_close(int fildes) { | ||
| track_closed_fd(fildes); | ||
| JUMP_INTO_LIBC_FUN(close, fildes); |
There was a problem hiding this comment.
Why do we need to use JUMP_INTO_LIBC_FUN for close but not for the others?
There was a problem hiding this comment.
We use it for all of them: the rest just call into a thunk that has the macro.
The reason is that the macro expands to a complex structure that ends in return. This means we can only use JUMP_INTO_LIBC_FUN as the last statement in a function. For close that's fine, we want to call the real close last, but for the others we want to call the real function first. Hence the thunk.
There was a problem hiding this comment.
We call the thunks in hooked-functions-unix.c but not in this file (hooked-functions-darwin.c), or do we?
Also JUMP_INTO_LIBC_FUN in this file just calls the function and returns the result.
There was a problem hiding this comment.
Apologies, you're right. The same answer applies though: we don't want the immediate return.
There was a problem hiding this comment.
I don't quite follow. Why can't we just write:
| JUMP_INTO_LIBC_FUN(close, fildes); | |
| return close(fildes); |
instead?
The other functions in this file appear to call the libc function without any thunks. Am I overlooking something?
There was a problem hiding this comment.
Consistency only.
The Unix implementations use JUMP_INTO_LIBC_FUN, and so this file does too. The other hooks all use it. However, the newly added hooks differ in that some of them care about the return value of the system call.
We don't have to use JUMP_INTO_LIBC_FUN here, and so we don't, but where it's possible to do so I wanted to preserve compatibility with the rest of the code in the file.
There was a problem hiding this comment.
hm... it is only halfway consistent because we are missing the thunks for accept, socket and in theory accept4. We don't share the code and we can't just copy and paste it over because behaviour differs slightly and of the missing thunks. JUMP_INTO_LIBC_FUN is even a no-op in this file so I'm wondering if the "consistency" here just makes everything harder to understand without any clear benefit.
But we can make it fully consistent and I think even share most of the code. However, this probably more effort than it is worth it. No hard feelings on that though after I have fully understood that this is just a no-op. Feel free to leave it as is.
Motivation
This patch extends the NIO integration test harness to track file descriptors, in particular to search for leaks. This change has been validated on Linux and Darwin, and in both cases correctly diagnoses FD leaks.
The goal is to enable us to regression test for things like
Modifications
Results
We can write regression tests for FD leaks.