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

Handle process termination during read from /proc #2816

Merged
merged 1 commit into from
May 17, 2017

Conversation

noidi
Copy link
Contributor

@noidi noidi commented May 16, 2017

Fixes #2815.

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)

@phemmer
Copy link
Contributor

phemmer commented May 16, 2017

This might be worth reporting upstream to GO that it should be handled by os.IsNotExist(). Yes os.IsNotExist() is typically only useful on Open(), but I think the semantics of the function apply here, as the ESRCH means that the file has disappeared.

@noidi
Copy link
Contributor Author

noidi commented May 16, 2017

Personally, I don't think os.IsNotExist() should check for ESRCH. Currently it only checks for ENOENT [1], which (at least on Linux) is reserved for path operations [2], so I think it's meant to detect errors caused by operations on non-existent paths. In this case the path does exist during open(), but the corresponding process is terminated while (or before) read() is being called on the opened file descriptor, causing something akin to an I/O error. (I think it's like removing a USB drive while a file is being read: the file becomes unreadable even though there's nothing wrong with the path that the program opened.)

[1] https://github.com/golang/go/blob/964639cc338db650ccadeafb7424bc8ebb2c0f6c/src/os/error_unix.go#L18
[2] https://lkml.org/lkml/2012/12/23/75

@danielnelson danielnelson merged commit c53d9fa into influxdata:master May 17, 2017
@danielnelson
Copy link
Contributor

Interesting issue, I always thought that if open succeeded you were good to read. It sounds like this could occur with any file in /proc/<pid>/, Are there any other files this could happen with?

@danielnelson
Copy link
Contributor

Whoops, I forgot to ask you to add it the changelog, I'll add it

@noidi
Copy link
Contributor Author

noidi commented May 18, 2017

@danielnelson Thanks for the merge! I don't have much experience with /proc/<pid>/ but I'd be surprised if the other files didn't work this way. I did some cursory grepping and I think this was the only place in the Telegraf source code where this could be a problem.

@noidi noidi deleted the issue-2815-no-such-process branch May 18, 2017 06:04
danielnelson added a commit that referenced this pull request May 19, 2017
@danielnelson danielnelson added this to the 1.3.1 milestone May 19, 2017
vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
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.

4 participants