Skip to content

[fix] handle close errors#1207

Closed
BridgeAR wants to merge 1 commit intowinstonjs:masterfrom
BridgeAR:master
Closed

[fix] handle close errors#1207
BridgeAR wants to merge 1 commit intowinstonjs:masterfrom
BridgeAR:master

Conversation

@BridgeAR
Copy link
Copy Markdown
Contributor

@BridgeAR BridgeAR commented Feb 14, 2018

Not using a callback is deprecated. Errors should properly be
handled and this a adds callback to do so.

Refs: nodejs/node#18668

Not using a callback is deprecated. Errors should properly be
handled and this adds callback to do so.
@BridgeAR
Copy link
Copy Markdown
Contributor Author

@indexzero PTAL.

@indexzero
Copy link
Copy Markdown
Member

This needs to be back-ported onto 2.x as well.

@indexzero
Copy link
Copy Markdown
Member

@BridgeAR thank you for your contribution. Going to close this in preference of #1227 and #1228. From the perspective of the caller any errors arising out of closing the file descriptor are safe to ignore since:

  1. The file has been opened successfully
  2. The file has been read successfully

If the permissions of the file change during that time then the error itself is probably not all that useful. Willing to reconsider this position if tests are provided illustrating how handling this error is important.

@indexzero indexzero closed this Mar 6, 2018
@BridgeAR
Copy link
Copy Markdown
Contributor Author

BridgeAR commented Mar 6, 2018

I am fine with #1227.

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

Successfully merging this pull request may close these issues.

3 participants