mcchrish / nnn.vim

File manager for vim/neovim powered by n³
BSD 2-Clause "Simplified" License
646 stars 25 forks source link

fix: send SIGTERM to job when trying to exit #125

Closed N-R-K closed 3 years ago

N-R-K commented 3 years ago

Fixes: https://github.com/mcchrish/nnn.vim/pull/123#issuecomment-947805683

N-R-K commented 3 years ago

@jarun can you confirm if this fixes the :qa issue on vim? On neovim the issue doesn't happen (at least I'm not able to reproduce it), so no changes should be needed.

N-R-K commented 3 years ago

And btw, this issue happens on NnnPicker as well. But I guess no one tried to do :qa while having Picker window open so it never got caught.

jarun commented 3 years ago

Yes, it's fixed.

However, I still see:

mkfifo: cannot create fifo '/tmp/vHsmzKA/0': File exists
N-R-K commented 3 years ago

However, I still see:

mkfifo: cannot create fifo '/tmp/vHsmzKA/0': File exists

Can't reproduce. Can you manually delete that file and see if the issue is reproducible on your end?

jarun commented 3 years ago

It's a new file everytime. But I noticed that I see this error if I just open the explorer (but not any new file using it). This is thrown by mkfifo. Are you trying to create a fifo that already exists?

N-R-K commented 3 years ago

This is thrown by mkfifo. Are you trying to create a fifo that already exists?

Fifo should be only created at :NnnExplorer startup.

https://github.com/mcchrish/nnn.vim/blob/d0f47ba44a4423754c122fd6e90b2dcc4e28f821/autoload/nnn.vim#L392

We're creating this ourselves to avoid the sleep hack. I saw @luukvbaal do it on nnn.nvim as well.

luukvbaal commented 3 years ago

I put it in the setup() function so it is only run once.

luukvbaal commented 3 years ago

I guess you should just put it at the top of ftplugin/nnn.vim or autoload/nnn.vim idk 🤷🏻‍♂️

N-R-K commented 3 years ago

We're deleting the fifo on explorer exit though. So don't think that should be the problem. I think maybe the fifo isn't getting deleted for some reason.

Will have to look deeper into this.

jarun commented 3 years ago

I don't see a leftover fifo in my tmp after I quit vim. Each time a random fifo is created and deleted when I quit vim.

After I open Explorer, with vi still open I can see the directory /tmp/vRTFHVf/. So I guess somehow we are attempting to create it twice. If nothing, can you try to make it silent?

N-R-K commented 3 years ago

@jarun can try the following patch and see if that works? I think we need to create the fifo before calling s:build_window otherwise nnn might be trying to create the fifo as well.

diff --git a/autoload/nnn.vim b/autoload/nnn.vim
index a883d3e..660cb4d 100644
--- a/autoload/nnn.vim
+++ b/autoload/nnn.vim
@@ -385,11 +385,11 @@ function! nnn#explorer(...) abort
     let l:opts.ppos = { 'buf': bufnr(''), 'win': winnr(), 'tab': tabpagenr() }
     let l:On_exit = s:explorer_create_on_exit_callback(l:opts)

+    " create the fifo ourselves since otherwise nnn might not create it on time
+    execute 'silent !mkfifo '.s:explorer_fifo
     let l:opts.term = s:build_window(l:layout, { 'cmd': l:cmd, 'on_exit': l:On_exit })
     let b:tbuf = l:opts.term

-    " create the fifo ourselves since otherwise nnn might not create it on time
-    execute 'silent !mkfifo '.s:explorer_fifo
     call s:explorer_job()

     autocmd BufEnter <buffer> startinsert
jarun commented 3 years ago

@N-R-K somehow I don't see the issue anymore so ignore it right now. If I see it again after reboot or something I will try this and update you.

Thanks a lot for checking this out and the awesome Explorer mode!!!

N-R-K commented 3 years ago

@jarun I can reproduce the issue now after adding a sleep 2 after s:build_window. This means nnn is creating the fifo before us. I'll open the PR with the fix.

jarun commented 3 years ago

Awesome!