Skip to content
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

Use open file descriptors to retain file between prepare and marshal phases #8

Merged
merged 1 commit into from Nov 27, 2014

Conversation

@kanongil
Copy link
Member

kanongil commented Oct 27, 2014

WIP patch to fix #4, including new & updated tests.

This patch seems to work as is, and passes all test on my machine. However, it doesn't have 100% code coverage due to never triggering the Fs.fstat() error response. Given that this is only likely to trigger on messed up filesystems with an EIO error, I'm at a loss on how to test for this. Perhaps some kind of mocking?

I also have concerns about forgetting open file descriptors. I do ensure that they are closed whenever finish is triggered on the response but I found no such hooks for error'ed responses, which is needed for this to work. Am I missing something?

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 3, 2014

Anyone that can help on the code coverage issue?

@kanongil kanongil force-pushed the kanongil:handle-file-changes-fix branch from 72b45d1 to 21eae25 Nov 4, 2014
@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 4, 2014

I found a workaround for the missing close event and added an extra test that attempts to provoke a file descriptor leak by aborting the response.

The only missing component to this patch is the code coverage. The best idea I have for this (besides a complicated mocking setup), is to collate the fstat error with the open error by using something like async.series(). Would this be acceptable?

@kanongil kanongil force-pushed the kanongil:handle-file-changes-fix branch from 21eae25 to 93256cf Nov 4, 2014
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 17, 2014

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 17, 2014

I will have a go at another test using mocking.

@hueniverse Any thoughts on the close hook? Is hapijs/hapi#2110 a possibility or can it be handled using promises in 8.0?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 17, 2014

Done.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 17, 2014

@kanongil how do you feel about taking over as lead maintainer of this module?

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 17, 2014

@hueniverse Thanks for the offer. It sounds like an interesting challenge, and I guess it would help you concentrate your efforts on the core of the project. What kind of commitment would you expect from me?

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 17, 2014

@hueniverse I guess this patch will have to wait a bit, as I am unable to install a suitable hapi@8.x to test against.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 17, 2014

This module is pretty low maintenance. You are the first in a long time to open issues against the file/directory handlers, and you seem eager to do the work so it isn't much more than getting to make the call about which changes go it, with someone else from the hapi core team doing a code review before merging. My guess is that it will stay a pretty low key module.

As for 8.0, you can manually link to the -rc1 modules in the shrinkwrap file, or give it a day and an 8.0-rc1 will be published.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 18, 2014

@hueniverse I'm in. Where do I sign up? Do you need something else from me?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 19, 2014

@kanongil you are now the lead maintainer and all setup on npm and github.

@kanongil kanongil force-pushed the kanongil:handle-file-changes-fix branch 2 times, most recently from 979a386 to dee3465 Nov 21, 2014
Fixes #4
@kanongil kanongil force-pushed the kanongil:handle-file-changes-fix branch from dee3465 to b1a0b4e Nov 26, 2014
@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 26, 2014

The patch is no longer WIP, and I would like it to be merged at some point. Any comments on the latest implementation?

My main concern is that the open file descriptors can get "lost" and never get closed. Assuming that hapi remembers to call close, this should only ever happen if a request is never reply()'ed to by a hook between the prepare and marshal phases. I'm inclined to think that this is mainly a theoretical concern but I'm open to feedback regarding this.

hueniverse added a commit that referenced this pull request Nov 27, 2014
Use open file descriptors to retain file between prepare and marshal phases
@hueniverse hueniverse merged commit cd5c016 into hapijs:master Nov 27, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@hueniverse hueniverse self-assigned this Nov 27, 2014
@hueniverse hueniverse added this to the 2.0.0 milestone Nov 27, 2014
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 27, 2014

Note that with this change, we now open fd for no reason when using pre-compressed files. I am not too concerned about it as the issue this fixes is more important but it does increase the number of fd created when errors are involved.

hueniverse added a commit that referenced this pull request Nov 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.