Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Fix file descriptor leak in the postoffice plugin #1989

Merged
merged 2 commits into from
Jun 23, 2020
Merged

Conversation

ip1981
Copy link
Member

@ip1981 ip1981 commented Jun 18, 2020

Closes #1838.

Signed-off-by: Igor Pashev pashev.igor@gmail.com


This change is Reviewable

@ip1981 ip1981 added the bug label Jun 18, 2020
@ip1981 ip1981 requested a review from a team June 18, 2020 19:11
@ip1981 ip1981 self-assigned this Jun 18, 2020
@ip1981
Copy link
Member Author

ip1981 commented Jun 18, 2020

But still, this line is not hit

tracing::debug!("Ending Inotify Listen for {}", &conf_file);

@ip1981 ip1981 force-pushed the ip1981/1838-leak branch 3 times, most recently from 8784f66 to c2da76d Compare June 18, 2020 19:18
Copy link
Contributor

@mkpankov mkpankov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ip1981)


iml-agent/src/daemon_plugins/postoffice.rs, line 183 at r1 (raw file):

Previously, ip1981 (Igor Pashev) wrote…

When all file descriptors referring to an inotify instance have been closed, the underlying object and its resources are freed for reuse by the kernel; all associated watches are automatically freed. --- https://linux.die.net/man/7/inotify

I'd expect inotify to close everything on drop, do you know why this is not the case?

@ip1981
Copy link
Member Author

ip1981 commented Jun 19, 2020

I'd expect inotify to close everything on drop, do you know why this is not the case?

I would postulate it is not dropped :)

@ip1981 ip1981 force-pushed the ip1981/1838-leak branch 2 times, most recently from 5f7bf30 to 0d576f0 Compare June 19, 2020 11:00
@nlinker
Copy link
Contributor

nlinker commented Jun 19, 2020

Since the issue is tricky, why not to add a description, how it is guaranteed now that the descriptors are freed?

@jgrund jgrund self-requested a review June 19, 2020 14:04
Copy link
Member

@jgrund jgrund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something doesn't make sense.

In:

https://github.com/jgrund/inotify/blob/b743b410afe91a9c161c9927bbac6ddd846a0253/src/fd_guard.rs#L50-L56

the same ffi::close fn is called iff close_on_drop is true.

So, this means either one of three things:

  1. Inotify is not being dropped because something else has a reference to it (have you tried using a valve on the listener stream?).
  2. close_on_drop is somehow false
  3. The ffi::close in drop is somehow behaving differently than the close method (unlikely).

We should figure this out instead of calling close explicitly as we're likely to use this crate elsewhere and may hit the same issue again.

@ip1981 ip1981 marked this pull request as draft June 19, 2020 14:13
The loop of inotify events was never ending
keeping a reference to the opened file descriptor.

On each new session a new file descriptor was allocated.
New session is created every time the manager (iml-http-agent) restarts.

Signed-off-by: Igor Pashev <pashev.igor@gmail.com>
@ip1981 ip1981 marked this pull request as ready for review June 22, 2020 19:01
@jgrund jgrund self-requested a review June 22, 2020 19:08
it is moved into the spawned task and kept alive as long as the task is.

Signed-off-by: Joe Grund <jgrund@whamcloud.io>
@jgrund jgrund requested a review from johnsonw June 22, 2020 19:24
@jgrund jgrund changed the title Fix file descritor leak in the postoffice plugin Fix file descriptor leak in the postoffice plugin Jun 22, 2020
@jgrund jgrund merged commit d245ebf into master Jun 23, 2020
@jgrund jgrund deleted the ip1981/1838-leak branch June 23, 2020 02:06
jgrund added a commit that referenced this pull request Jun 23, 2020
* Fix file descritor leak in the postoffice plugin

The loop of inotify events was never ending
keeping a reference to the opened file descriptor.

On each new session a new file descriptor was allocated.
New session is created every time the manager (iml-http-agent) restarts.

Signed-off-by: Igor Pashev <pashev.igor@gmail.com>

* Remove storage of inotify ref

it is moved into the spawned task and kept alive as long as the task is.

Signed-off-by: Joe Grund <jgrund@whamcloud.io>

Co-authored-by: Joe Grund <jgrund@whamcloud.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
5 participants