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

Allow Exec() without init the executable name #9

Merged
merged 1 commit into from Jan 4, 2020

Conversation

@sesquipedalian-dev
Copy link
Contributor

sesquipedalian-dev commented Jan 3, 2020

We use this library in a project (https://github.com/checkr/openmock) that uses go templates, and it's very handy to watch for the template files to change so we can reload everything. We also have an API interface that lets you manually set the same models, so it's convenient to store those in redis and then Exec() to reload the whole process. We've found that in local development, we run into problems with the file handles opened by reload don't get dropped when you restart the process, and after a few reloads it crashes with an error like:

FATA[0000] cannot add "/Users/sesquipedalian.dev/Documents/go_repos/src/github.com/checkr/openmock" to watcher: too many open files 

So, we added a configuration that lets you skip the initialization of reload (reload.Do), but in this case reload.Exec crashes because binSelf has not been set. This PR makes it so Exec will just figure out the executable name when called instead of needing to be initialized. This is handy if you want to use this 'crash-only software' style of reloading your program's data.

@codecov

This comment was marked as off-topic.

Copy link

codecov bot commented Jan 3, 2020

Codecov Report

Merging #9 into master will decrease coverage by 1.86%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   30.43%   28.57%   -1.87%     
==========================================
  Files           2        2              
  Lines          92       98       +6     
==========================================
  Hits           28       28              
- Misses         54       60       +6     
  Partials       10       10
Impacted Files Coverage Δ
reload.go 31.81% <0%> (-2.33%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d02e61a...d167ec3. Read the comment docs.

@arp242

This comment has been minimized.

Copy link
Collaborator

arp242 commented Jan 3, 2020

This seems like a reasonable patch. I'll look over it tomorrow with a fresh mind :-)

Aside from this PR, the resource leak should also be fixed. I confirmed with lsof that on every restart file handlers are being leaked, after two restarts I have these extra entries:

COMMAND     PID   USER   FD      TYPE   DEVICE SIZE/OFF     NODE NAME
goatcount 27629 martin    5u  a_inode     0,14        0     1037 [eventpoll]
goatcount 27629 martin    9u  a_inode     0,14        0     1037 [eventpoll]
goatcount 27629 martin   10r     FIFO     0,13      0t0 13673517 pipe
goatcount 27629 martin   11w     FIFO     0,13      0t0 13673517 pipe
goatcount 27629 martin   12u  a_inode     0,14        0     1037 [eventpoll]
goatcount 27629 martin   13r     FIFO     0,13      0t0 13672822 pipe
goatcount 27629 martin   14w     FIFO     0,13      0t0 13672822 pipe
goatcount 27629 martin   15r     FIFO     0,13      0t0 13673844 pipe
goatcount 27629 martin   16w     FIFO     0,13      0t0 13673844 pipe

I assumed that restarting the process would clean all of this, like existing a process would, but it seems not.

calling watcher.Close() in Exec() seems to fix this. Not sure if that's sufficient, will it also leak file handles from the program itself? 🤔 I suspect that's the problem you're running in to.

@sesquipedalian-dev

This comment has been minimized.

Copy link
Contributor Author

sesquipedalian-dev commented Jan 3, 2020

Thanks arp242 for taking a look! It would be great to fix the file leaks as well. Calling watcher.Close() makes sense; another way I found is CloseOnExec in the syscall library (https://golang.org/src/syscall/exec_unix.go?s=3661:3685#L93), but one would have to dig in to notify and find the file descriptors it uses. Just another thought on how to solve.

@arp242
arp242 approved these changes Jan 4, 2020
@arp242 arp242 merged commit 2cb9705 into Teamwork:master Jan 4, 2020
0 of 2 checks passed
0 of 2 checks passed
codecov/patch 0% of diff hit (target 30.43%)
Details
codecov/project 28.57% (-1.87%) compared to d02e61a
Details
@arp242

This comment has been minimized.

Copy link
Collaborator

arp242 commented Jan 4, 2020

Thanks for the PR; I've merged it now. I'll look at the leak in more depth later and fix it before I make a new version.

arp242 added a commit that referenced this pull request Jan 4, 2020
See #9
@arp242

This comment has been minimized.

Copy link
Collaborator

arp242 commented Jan 4, 2020

The leak should be fixed now (see linked commit) and I released v1.3.0. Let me know if you see any more problems, and thanks for reporting the issue (I've been using this library pretty much daily for more than 2 years, and never noticed it 🤔)

@sesquipedalian-dev

This comment has been minimized.

Copy link
Contributor Author

sesquipedalian-dev commented Jan 6, 2020

Thanks for fixing that!

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.