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

No closeSync API called #2

Open
xiabin1235910 opened this issue Nov 8, 2017 · 2 comments
Open

No closeSync API called #2

xiabin1235910 opened this issue Nov 8, 2017 · 2 comments

Comments

@xiabin1235910
Copy link

xiabin1235910 commented Nov 8, 2017

I found in helper.js, we open the file to get the fileDescriptor using fs.openSync(filePath, 'r').
But do we need to close it when the server is closed?

the code in the node official API

server.on('close', () => fs.closeSync(fd));

The link is
https://nodejs.org/dist/latest-v9.x/docs/api/http2.html#http2_http2stream_respondwithfd_fd_headers_options

@xiabin1235910
Copy link
Author

like in helper.js

function closeFiles (baseDir) {
    ...
}

@Shadowbeetle
Copy link
Member

Sorry for the late response. In case you're still interested:

That could be a problem if we wrote to the files, or if we opened many different files for different endpoints and ran out of Unix sockets. Neither if the two happens in this.

This solution is a bit more efficient as we open the files when the server is fired up, so we can read the file contents straight away upon receiving a request. You are right, however, that it would be safer and nicer to only open the files when they are needed and close them once their content is sent.

Would you be up for opening a pull request to fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants