From ff5867ed1083bc01dbf39035a4c29a2c777c6a95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Bortl=C3=ADk?= Date: Mon, 7 Sep 2026 12:15:56 +0200 Subject: [PATCH 1/3] feat: add keybinding for closing review The `gitlab.close_review` function now takes an optional `shut_down_server` parameter (Boolean, `true` by default) which can be used to specify if the Go server should be shut down when closing the review. --- README.md | 1 + doc/gitlab.nvim.txt | 12 +++++++++--- lua/gitlab/annotations.lua | 3 +++ lua/gitlab/init.lua | 5 +++-- lua/gitlab/reviewer/init.lua | 12 +++++++++--- lua/gitlab/state.lua | 7 +++++++ 6 files changed, 32 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 7921f023..9166d227 100644 --- a/README.md +++ b/README.md @@ -178,6 +178,7 @@ These keymaps are available globally (i.e., in any buffer). | `glC` | Create a new MR for currently checked-out feature branch | | `glc` | Chose MR for review | | `glS` | Start review for the currently checked-out branch | +| `glQ` | Close the reviewer, discussions, and shut down the Go server | | `gl` | Load new MR state from Gitlab and apply new diff refs to the diff view | | `gls` | Show the editable summary of the MR | | `glu` | Copy the URL of the MR to the system clipboard | diff --git a/doc/gitlab.nvim.txt b/doc/gitlab.nvim.txt index f5d1c9ec..936ca76a 100644 --- a/doc/gitlab.nvim.txt +++ b/doc/gitlab.nvim.txt @@ -256,6 +256,7 @@ you call this function with no values the defaults will be used: choose_merge_request = "glc", -- Chose MR for review (if necessary check out the feature branch) start_review = "glS", -- Start review for the currently checked-out branch reload_review = "gl", -- Load new MR state from Gitlab and apply new diff refs to the diff view + close_review = "glQ", -- Close the reviewer, discussions, and shut down the Go server summary = "gls", -- Show the editable summary of the MR copy_mr_url = "glu", -- Copy the URL of the MR to the system clipboard open_in_browser = "glo", -- Open the URL of the MR in the default Internet browser @@ -936,11 +937,16 @@ https://github.com/sindrets/diffview.nvim) closes and re-opens the reviewer. *gitlab.nvim.close_review* gitlab.close_review() ~ -Closes the reviewer tab and discussion tree and cleans up (e.g., removes -winbar timer). +Closes the reviewer tab and discussion tree, cleans up (e.g., removes +winbar timer), and optionally shuts down the Go server. >lua - require("gitlab").close_review() + require("gitlab").close_review({shut_down_server = false}) < + Parameters: ~ + • {opts}: (table?) + • {shut_down_server}: (`boolean?`, default: `true`) If true, the + Go server is shut down. + *gitlab.nvim.summary* gitlab.summary() ~ diff --git a/lua/gitlab/annotations.lua b/lua/gitlab/annotations.lua index 96f9b431..f3fe621b 100644 --- a/lua/gitlab/annotations.lua +++ b/lua/gitlab/annotations.lua @@ -11,6 +11,9 @@ ---@field avatar_url string ---@field web_url string +---@class CloseReviewerOpts +---@field shut_down_server boolean Whether the server should be shut down (default: true) + ---The modification of a line in a diff. ---@alias ModificationType ---| "old" A deleted line diff --git a/lua/gitlab/init.lua b/lua/gitlab/init.lua index a6412a21..09b15f72 100644 --- a/lua/gitlab/init.lua +++ b/lua/gitlab/init.lua @@ -75,8 +75,9 @@ return { reload_review = function() reviewer.reload() end, - close_review = function() - reviewer.close() + ---@param opts? CloseReviewerOpts + close_review = function(opts) + reviewer.close(opts) end, pipeline = async.sequence({ latest_pipeline }, pipeline.open), merge = async.sequence({ u.merge(info, { refresh = true }) }, merge.merge), diff --git a/lua/gitlab/reviewer/init.lua b/lua/gitlab/reviewer/init.lua index 45b097fb..301b61cd 100644 --- a/lua/gitlab/reviewer/init.lua +++ b/lua/gitlab/reviewer/init.lua @@ -5,6 +5,7 @@ local List = require("gitlab.utils.list") local u = require("gitlab.utils") +local server = require("gitlab.server") local state = require("gitlab.state") local async = require("diffview.async") @@ -92,8 +93,10 @@ M.open = function() git.check_mr_in_good_condition() end ----Close the reviewer and clean up. -M.close = function() +---Close the reviewer, clean up, and shut down the Go server. +---@param opts? CloseReviewerOpts +M.close = function(opts) + opts = u.merge({ shut_down_server = true }, opts and opts or {}) if M.tabid ~= nil and vim.api.nvim_tabpage_is_valid(M.tabid) then -- FIXME: This fails if there is only one tabpage. Find a way to use DiffviewClose -- that was originally here, but use it for the correct tabpage when there are @@ -102,6 +105,9 @@ M.close = function() end local discussions = require("gitlab.actions.discussions") discussions.close() + if opts.shut_down_server then + server.shutdown() + end end ---Load new INFO state from Gitlab. Then, if diffview.api is available, apply the new @@ -116,7 +122,7 @@ M.reload = function() { view = M.diffview } ) else - M.close() + M.close({ shut_down_server = false }) M.open() end end) diff --git a/lua/gitlab/state.lua b/lua/gitlab/state.lua index 0d25b264..6f53607a 100644 --- a/lua/gitlab/state.lua +++ b/lua/gitlab/state.lua @@ -108,6 +108,7 @@ M.settings = { choose_merge_request = "glc", start_review = "glS", reload_review = "gl", + close_review = "glQ", summary = "gls", copy_mr_url = "glu", open_in_browser = "glo", @@ -348,6 +349,12 @@ M.set_global_keymaps = function() end, { desc = "Reload Gitlab review", nowait = keymaps.global.reload_review_nowait }) end + if keymaps.global.close_review then + vim.keymap.set("n", keymaps.global.close_review, function() + require("gitlab").close_review() + end, { desc = "Close Gitlab review", nowait = keymaps.global.close_review_nowait }) + end + if keymaps.global.choose_merge_request then vim.keymap.set("n", keymaps.global.choose_merge_request, function() require("gitlab").choose_merge_request() From 757c8560c1978d884eff534eeeafae20b102dd49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Bortl=C3=ADk?= Date: Mon, 7 Sep 2026 12:15:56 +0200 Subject: [PATCH 2/3] fix: kill Go server on Neovim exit A VimLeavePre autocmd is registered that kills the server when Neovim is terminating. --- lua/gitlab/actions/discussions/init.lua | 12 ++++++++ lua/gitlab/async.lua | 4 +-- lua/gitlab/autocmd.lua | 1 + lua/gitlab/colors.lua | 3 ++ lua/gitlab/emoji.lua | 7 ++++- lua/gitlab/popup.lua | 5 +++ lua/gitlab/reviewer/init.lua | 15 ++++++--- lua/gitlab/server.lua | 41 ++++++++++++++++++++++--- 8 files changed, 76 insertions(+), 12 deletions(-) create mode 100644 lua/gitlab/autocmd.lua diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index 70a952dc..32c0cc88 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -21,6 +21,7 @@ local diagnostics = require("gitlab.indicators.diagnostics") local winbar = require("gitlab.actions.discussions.winbar") local help = require("gitlab.actions.help") local emoji = require("gitlab.emoji") +local GitlabGroup = require("gitlab.autocmd") local M = { split_visible = false, @@ -156,12 +157,16 @@ M.open = function(callback, view_type) callback = function() M.linked_bufnr = nil end, + desc = "Nil the linked discussion buffer", + group = GitlabGroup, }) vim.api.nvim_create_autocmd("BufWipeout", { buffer = M.unlinked_bufnr, callback = function() M.unlinked_bufnr = nil end, + desc = "Nil the unlinked discussion buffer", + group = GitlabGroup, }) -- Set autocmd to clean up state when discussions split is closed manually @@ -172,6 +177,8 @@ M.open = function(callback, view_type) callback = function() vim.schedule(M.close) end, + desc = "Clear the discussion state", + group = GitlabGroup, }) -- Initialize winbar @@ -608,13 +615,18 @@ M.create_split_and_bufs = function() M.last_row, M.last_column = unpack(vim.api.nvim_win_get_cursor(0)) M.last_node_at_cursor = M.discussion_tree and M.discussion_tree:get_node() or nil end, + desc = "Store cursor position", + group = GitlabGroup, }) vim.api.nvim_create_autocmd("WinLeave", { buffer = unlinked_bufnr, callback = function() + -- TODO: Shouldn't this also set last_row and last_column? M.last_node_at_cursor = M.unlinked_discussion_tree and M.unlinked_discussion_tree:get_node() or nil end, + desc = "Store cursor position", + group = GitlabGroup, }) return split, linked_bufnr, unlinked_bufnr diff --git a/lua/gitlab/async.lua b/lua/gitlab/async.lua index 5cef1a68..37f20136 100644 --- a/lua/gitlab/async.lua +++ b/lua/gitlab/async.lua @@ -72,8 +72,8 @@ end ---Sets plugin configuration and builds and starts the server if necessary. ---@generic T ---@param dependencies GitlabDependency[] ----@param cb fun(argrs: T) ----@return fun(argrs: T) +---@param cb fun(args: T?) +---@return fun(args: T?) M.sequence = function(dependencies, cb) return function(args) local handler = async:new() diff --git a/lua/gitlab/autocmd.lua b/lua/gitlab/autocmd.lua new file mode 100644 index 00000000..f6dd95fd --- /dev/null +++ b/lua/gitlab/autocmd.lua @@ -0,0 +1 @@ +return vim.api.nvim_create_augroup("GitlabGroup", {}) diff --git a/lua/gitlab/colors.lua b/lua/gitlab/colors.lua index bf29dec3..3031f677 100644 --- a/lua/gitlab/colors.lua +++ b/lua/gitlab/colors.lua @@ -1,4 +1,5 @@ local state = require("gitlab.state") +local GitlabGroup = require("gitlab.autocmd") -- Set icons into global vim variables for syntax matching -- TODO: This could be simplified to assigning the discussion_tree table to @@ -36,4 +37,6 @@ vim.api.nvim_create_autocmd({ "VimEnter", "ColorScheme" }, { vim.api.nvim_set_hl(0, "GitlabLiveMode", get_colors_for_group(discussion_colors.live_mode)) vim.api.nvim_set_hl(0, "GitlabSortMethod", get_colors_for_group(discussion_colors.sort_method)) end, + desc = "Set up highlight groups", + group = GitlabGroup, }) diff --git a/lua/gitlab/emoji.lua b/lua/gitlab/emoji.lua index 9d8e5daa..04949874 100644 --- a/lua/gitlab/emoji.lua +++ b/lua/gitlab/emoji.lua @@ -1,5 +1,6 @@ local u = require("gitlab.utils") local common = require("gitlab.actions.common") +local GitlabGroup = require("gitlab.autocmd") -- Basic emoji aliases that are missing in Gitlab's list. ---@type Emoji @@ -91,6 +92,7 @@ end ---@param bufnr integer The number of the buffer that holds the discussion tree M.init_popup = function(tree, bufnr) vim.api.nvim_create_autocmd({ "CursorHold" }, { + buffer = bufnr, callback = function() local node = tree:get_node() if node == nil or not common.is_node_note(node) then @@ -132,7 +134,8 @@ M.init_popup = function(tree, bufnr) end end end, - buffer = bufnr, + desc = "Open the emoji popup", + group = GitlabGroup, }) vim.api.nvim_create_autocmd({ "CursorMoved", "CursorMovedI" }, { @@ -140,6 +143,8 @@ M.init_popup = function(tree, bufnr) M.close_popup() end, buffer = bufnr, + desc = "Close the emoji popup", + group = GitlabGroup, }) end diff --git a/lua/gitlab/popup.lua b/lua/gitlab/popup.lua index 3ab7c95f..85429185 100644 --- a/lua/gitlab/popup.lua +++ b/lua/gitlab/popup.lua @@ -1,4 +1,5 @@ local u = require("gitlab.utils") +local GitlabGroup = require("gitlab.autocmd") local M = {} @@ -131,6 +132,8 @@ M.set_popup_keymaps = function(popup, action, linewise_action, opts) vim.fn.setreg(register, text) end end, + desc = "Save popup contents to temp registers.", + group = GitlabGroup, }) end @@ -140,6 +143,8 @@ M.set_popup_keymaps = function(popup, action, linewise_action, opts) callback = function() exit(popup, opts) end, + desc = "Run callback before exiting popup", + group = GitlabGroup, }) end end diff --git a/lua/gitlab/reviewer/init.lua b/lua/gitlab/reviewer/init.lua index 301b61cd..b55d1091 100644 --- a/lua/gitlab/reviewer/init.lua +++ b/lua/gitlab/reviewer/init.lua @@ -267,12 +267,13 @@ M.set_callback_for_file_changed = function(callback) local group = vim.api.nvim_create_augroup("gitlab.diffview.autocommand.file_changed", {}) vim.api.nvim_create_autocmd("User", { pattern = { "DiffviewDiffBufWinEnter" }, - group = group, callback = function(...) if M.tabid == vim.api.nvim_get_current_tabpage() then callback(...) end end, + desc = "Run callback when Diffview file changes", + group = group, }) end @@ -282,7 +283,6 @@ M.set_callback_for_buf_read = function(callback) local group = vim.api.nvim_create_augroup("gitlab.diffview.autocommand.buf_read", {}) vim.api.nvim_create_autocmd("User", { pattern = { "DiffviewDiffBufRead" }, - group = group, callback = function(...) -- Only run the callback when we're in the MR's tabpage or when the tabpage has -- not yet been set (tabid = nil) in a freshly started review (is_open = true). @@ -292,6 +292,8 @@ M.set_callback_for_buf_read = function(callback) callback(...) end end, + desc = "Run callback when Diffview buffer is loaded", + group = group, }) end @@ -301,12 +303,13 @@ M.set_callback_for_reviewer_leave = function(callback) local group = vim.api.nvim_create_augroup("gitlab.diffview.autocommand.leave", {}) vim.api.nvim_create_autocmd("User", { pattern = { "DiffviewViewLeave", "DiffviewViewClosed" }, - group = group, callback = function(...) if vim.api.nvim_get_current_tabpage() == M.tabid then callback(...) end end, + desc = "Run callback when focus leaves Diffview", + group = group, }) end @@ -317,12 +320,13 @@ M.set_callback_for_reviewer_enter = function(callback) local group = vim.api.nvim_create_augroup("gitlab.diffview.autocommand.enter", {}) vim.api.nvim_create_autocmd("User", { pattern = { "DiffviewViewEnter", "DiffviewViewOpened" }, - group = group, callback = function(...) if vim.api.nvim_get_current_tabpage() == M.tabid then callback(...) end end, + desc = "Run callback when focus enters Diffview", + group = group, }) end @@ -477,7 +481,6 @@ end M.set_reviewer_autocommands = function(bufnr) local group = vim.api.nvim_create_augroup("gitlab.diffview.autocommand.win_enter." .. bufnr, {}) vim.api.nvim_create_autocmd({ "WinEnter", "BufWinEnter" }, { - group = group, buffer = bufnr, callback = function() if vim.api.nvim_get_current_win() == M.buf_winids[bufnr] then @@ -492,6 +495,8 @@ M.set_reviewer_autocommands = function(bufnr) del_keymaps(bufnr) end end, + desc = "(Un)set buffer-local options for reviewer buffers", + group = group, }) end diff --git a/lua/gitlab/server.lua b/lua/gitlab/server.lua index 70b3fd87..ff04766e 100644 --- a/lua/gitlab/server.lua +++ b/lua/gitlab/server.lua @@ -5,9 +5,18 @@ local state = require("gitlab.state") local u = require("gitlab.utils") local client = require("gitlab.client") local version = require("gitlab.version") +local GitlabGroup = require("gitlab.autocmd") local M = {} +---@type vim.SystemObj? +local server_system_obj +local kill_autocmd_created = false +-- Set when the VimLeavePre autocmd kills the server, so that the server's +-- on_exit callback skips the "server exited" error notification: it would +-- otherwise fire (and might fail to render) while nvim is tearing down. +local killed_on_exit = false + -- Builds the binary if it doesn't exist, and starts the server. If the pre-existing binary has an older -- tag than the Lua code (exposed via the /version endpoint) then shuts down the server, rebuilds it, and -- restarts the server again. @@ -63,7 +72,29 @@ M.start = function(callback) local settings = vim.json.encode(go_server_settings) - local ok, err = pcall(vim.system, { state.settings.server.binary, settings }, { + if not kill_autocmd_created then + kill_autocmd_created = true + vim.api.nvim_create_autocmd({ "VimLeavePre" }, { + callback = function() + if server_system_obj == nil then + return + end + killed_on_exit = true + -- Send SIGKILL rather than SIGTERM: the server is a stateless proxy to the + -- Gitlab API, and a graceful shutdown would wait for in-flight requests. + -- Use `pcall` because the handle is already closed if the process exited on + -- its own, e.g. through the /shutdown endpoint. + pcall(function() + server_system_obj:kill("sigkill") + end) + server_system_obj = nil + end, + desc = "Kill the gitlab.nvim Go server process", + group = GitlabGroup, + }) + end + + local ok, obj = pcall(vim.system, { state.settings.server.binary, settings }, { stdout = function(_, data) if data == nil or parsed_port ~= nil then return @@ -85,7 +116,7 @@ M.start = function(callback) end end, }, function(out) - if out.code ~= 0 then + if out.code ~= 0 and not killed_on_exit then vim.schedule(function() local msg = "Golang gitlab server exited: code: " .. out.code .. ", signal: " .. (out.signal or 0) if out.stderr ~= "" then @@ -96,8 +127,10 @@ M.start = function(callback) end end) - if not ok then - u.notify("Could not start gitlab.nvim binary: " .. tostring(err), vim.log.levels.ERROR) + if ok then + server_system_obj = obj + else + u.notify("Could not start gitlab.nvim binary: " .. tostring(obj), vim.log.levels.ERROR) end end From 307069fe080bf983d36b8df245e69b1f6810874c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Bortl=C3=ADk?= Date: Mon, 7 Sep 2026 12:15:56 +0200 Subject: [PATCH 3/3] fix: kill Go server when Neovim terminates unexpectedly The plugin now opens a pipe on the server's stdin which let's the server notice that the parent Neovim process is gone and exit in cases that are not covered by the VimLeavePre autocmd which shuts down the server when Neovim terminates normally. --- cmd/app/shutdown.go | 67 ++++++++- cmd/app/shutdown_test.go | 289 +++++++++++++++++++++++++++++++++++++++ cmd/main.go | 4 + lua/gitlab/server.lua | 5 + 4 files changed, 362 insertions(+), 3 deletions(-) create mode 100644 cmd/app/shutdown_test.go diff --git a/cmd/app/shutdown.go b/cmd/app/shutdown.go index 043b232e..e61bd76a 100644 --- a/cmd/app/shutdown.go +++ b/cmd/app/shutdown.go @@ -4,10 +4,18 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "os" + "time" ) +/* +How long to wait for in-flight requests to finish before shutting down anyway. +Without a deadline a slow Gitlab call keeps the process alive after Neovim quits. +*/ +const shutdownTimeout = 10 * time.Second + type killer struct{} func (k killer) Signal() {} @@ -22,18 +30,71 @@ type ShutdownHandler interface { type shutdownService struct { sigCh chan os.Signal + /* How long to wait for in-flight requests. Zero means shutdownTimeout; tests + shrink it so they do not have to wait out the real deadline. */ + timeout time.Duration } func (s shutdownService) WatchForShutdown(server *http.Server) { /* Handles shutdown requests */ <-s.sigCh - err := server.Shutdown(context.Background()) - if err != nil { + + timeout := s.timeout + if timeout == 0 { + timeout = shutdownTimeout + } + + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + /* Not fatal: exiting 0 keeps the Lua side from reporting the shutdown as a + crash. */ + if err := server.Shutdown(ctx); err != nil { fmt.Fprintf(os.Stderr, "Server could not shut down gracefully: %s\n", err) - os.Exit(1) } } +/* +WatchForParentExit exits the process once stdin reaches EOF, which happens when +Neovim closes its end of the pipe, i.e. when it dies. This is the only cleanup +that survives a SIGKILL or a crash, where neither /shutdown nor the VimLeavePre +autocmd gets to run. The kernel closes the pipe no matter how the parent died. + +The callback exits immediately rather than waiting for running requests to finish, +as /shutdown does. Neovim is already gone, so nobody would see those responses; +waiting could keep the process alive for up to shutdownTimeout for no reason. +*/ +func WatchForParentExit() { + if !isPipeLike(os.Stdin) { + return + } + + watchForEOF(os.Stdin, func() { os.Exit(0) }) +} + +/* +isPipeLike reports whether EOF on the file would really mean that Neovim is gone. +Sockets count too: libuv connects the child's stdin with a socketpair, not a FIFO. +When the server is started by hand, stdin is a terminal or /dev/null, where EOF +means a Ctrl-D or nothing at all. +*/ +func isPipeLike(f *os.File) bool { + info, err := f.Stat() + if err != nil { + return false + } + + return info.Mode()&(os.ModeNamedPipe|os.ModeSocket) != 0 +} + +/* watchForEOF calls onEOF, in a goroutine, once r is exhausted or errors. */ +func watchForEOF(r io.Reader, onEOF func()) { + go func() { + _, _ = io.Copy(io.Discard, r) + onEOF() + }() +} + type ShutdownRequest struct { Restart bool `json:"restart"` } diff --git a/cmd/app/shutdown_test.go b/cmd/app/shutdown_test.go new file mode 100644 index 00000000..534dbda4 --- /dev/null +++ b/cmd/app/shutdown_test.go @@ -0,0 +1,289 @@ +package app + +import ( + "fmt" + "net" + "net/http" + "os" + "sync" + "syscall" + "testing" + "time" +) + +/* +WatchForShutdown must not wait for an in-flight request forever. Before the +shutdown context had a deadline, a single request still waiting on Gitlab kept +the process alive after Neovim had already quit. +*/ +func TestWatchForShutdownGivesUpOnInFlightRequests(t *testing.T) { + /* Blocks until the test is over, standing in for a slow Gitlab call */ + handlerDone := make(chan struct{}) + defer close(handlerDone) + + requestReceived := make(chan struct{}) + + server := &http.Server{ + Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + close(requestReceived) + <-handlerDone + }), + } + + listener, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatal(err) + } + go func() { + _ = server.Serve(listener) + }() + defer func() { _ = server.Close() }() + + url := fmt.Sprintf("http://%s/", listener.Addr().String()) + go func() { + resp, err := http.Get(url) + if err == nil { + _ = resp.Body.Close() + } + }() + + /* Only shut down once the handler is actually running, otherwise the server + would be idle and would shut down immediately whether it is bounded or not. */ + select { + case <-requestReceived: + case <-time.After(5 * time.Second): + t.Fatal("Handler never received the request") + } + + s := shutdownService{sigCh: make(chan os.Signal, 1), timeout: 100 * time.Millisecond} + s.sigCh <- killer{} + + returned := make(chan time.Duration, 1) + go func() { + start := time.Now() + s.WatchForShutdown(server) + returned <- time.Since(start) + }() + + select { + case elapsed := <-returned: + /* A lower bound alone would also pass if the function blocked for 100ms for + some unrelated reason. The upper bound shows that it was the deadline that + released the wait, not some other delay. */ + if elapsed < s.timeout { + t.Errorf("Returned after %v, before the %v deadline", elapsed, s.timeout) + } + if elapsed > time.Second { + t.Errorf("Returned after %v, long past the %v deadline: it did not give up on the request", elapsed, s.timeout) + } + case <-time.After(5 * time.Second): + t.Fatal("WatchForShutdown did not return: it is still waiting for the request") + } +} + +/* An idle server should still shut down straight away, well inside the deadline. */ +func TestWatchForShutdownReturnsImmediatelyWhenIdle(t *testing.T) { + server := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})} + + listener, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatal(err) + } + go func() { + _ = server.Serve(listener) + }() + + s := shutdownService{sigCh: make(chan os.Signal, 1), timeout: 5 * time.Second} + s.sigCh <- killer{} + + returned := make(chan struct{}) + go func() { + s.WatchForShutdown(server) + close(returned) + }() + + select { + case <-returned: + case <-time.After(2 * time.Second): + t.Fatal("Idle server did not shut down promptly") + } +} + +/* +The default timeout applies when the service does not set one. If the fallback +is dropped, the context expires immediately and Shutdown gives up before the +in-flight request finishes. +*/ +func TestWatchForShutdownDefaultsToShutdownTimeout(t *testing.T) { + handlerDone := make(chan struct{}) + requestReceived := make(chan struct{}) + + server := &http.Server{ + Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + close(requestReceived) + <-handlerDone + }), + } + + listener, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatal(err) + } + go func() { + _ = server.Serve(listener) + }() + defer func() { _ = server.Close() }() + + url := fmt.Sprintf("http://%s/", listener.Addr().String()) + go func() { + resp, err := http.Get(url) + if err == nil { + _ = resp.Body.Close() + } + }() + + select { + case <-requestReceived: + case <-time.After(5 * time.Second): + t.Fatal("Handler never received the request") + } + + var once sync.Once + release := func() { once.Do(func() { close(handlerDone) }) } + /* Lets the handler finish shortly after the shutdown starts: the fallback + keeps the context alive long enough for Shutdown to wait it out. */ + time.AfterFunc(200*time.Millisecond, release) + defer release() + + s := shutdownService{sigCh: make(chan os.Signal, 1)} + assert(t, s.timeout, time.Duration(0)) + assert(t, shutdownTimeout, 10*time.Second) + s.sigCh <- killer{} + + start := time.Now() + s.WatchForShutdown(server) + elapsed := time.Since(start) + + if elapsed < 100*time.Millisecond { + t.Errorf("Returned after %v, before the handler finished: the context expired immediately, so the default timeout was not applied", elapsed) + } +} + +/* EOF on the pipe means the process that spawned the server is gone. */ +func TestWatchForEOFFiresWhenTheWriteEndCloses(t *testing.T) { + reader, writer, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + fired := make(chan struct{}) + watchForEOF(reader, func() { close(fired) }) + + select { + case <-fired: + t.Fatal("Fired while the write end was still open") + case <-time.After(200 * time.Millisecond): + } + + if err := writer.Close(); err != nil { + t.Fatal(err) + } + + select { + case <-fired: + case <-time.After(5 * time.Second): + t.Fatal("Did not fire after the write end closed") + } +} + +/* +The parent may die between the process starting and the watch beginning: EOF +that is already pending when watchForEOF starts must still fire. +*/ +func TestWatchForEOFFiresOnAlreadyClosedPipe(t *testing.T) { + reader, writer, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + /* Simulate that Neovim dies before the EOF watch even starts */ + defer func() { _ = reader.Close() }() + if err := writer.Close(); err != nil { + t.Fatal(err) + } + + fired := make(chan struct{}) + watchForEOF(reader, func() { close(fired) }) + + select { + case <-fired: + case <-time.After(5 * time.Second): + t.Fatal("Did not fire on an already-exhausted pipe") + } +} + +/* Data on stdin must not be mistaken for the parent going away. */ +func TestWatchForEOFIgnoresWrites(t *testing.T) { + reader, writer, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + defer func() { _ = writer.Close() }() + + fired := make(chan struct{}) + watchForEOF(reader, func() { close(fired) }) + + if _, err := writer.WriteString("some noise\n"); err != nil { + t.Fatal(err) + } + + select { + case <-fired: + t.Fatal("Fired on a write rather than on EOF") + case <-time.After(200 * time.Millisecond): + } +} + +/* +Only a pipe means "spawned by the plugin". Started by hand, stdin is a terminal +or /dev/null, and EOF there must not shut the server down. +*/ +func TestIsPipeLike(t *testing.T) { + reader, writer, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + defer func() { _ = writer.Close() }() + assert(t, isPipeLike(reader), true) + + devNull, err := os.Open(os.DevNull) + if err != nil { + t.Fatal(err) + } + defer func() { _ = devNull.Close() }() + assert(t, isPipeLike(devNull), false) + + regular, err := os.CreateTemp(t.TempDir(), "stdin") + if err != nil { + t.Fatal(err) + } + defer func() { _ = regular.Close() }() + assert(t, isPipeLike(regular), false) +} + +/* Neovim connects the child's stdin with a socketpair, not with a FIFO. */ +func TestIsPipeLikeAcceptsSocketpair(t *testing.T) { + fds, err := syscall.Socketpair(syscall.AF_UNIX, syscall.SOCK_STREAM, 0) + if err != nil { + t.Fatal(err) + } + + local := os.NewFile(uintptr(fds[0]), "socketpair") + defer func() { _ = local.Close() }() + remote := os.NewFile(uintptr(fds[1]), "socketpair") + defer func() { _ = remote.Close() }() + + assert(t, isPipeLike(local), true) +} diff --git a/cmd/main.go b/cmd/main.go index 3aae8dbe..201d7df3 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -15,6 +15,10 @@ var Version = "unknown" // Set via ldflags func main() { log.SetFlags(0) + /* Set up before anything else, so that a server that is still initializing is + also shut down when Neovim dies. */ + app.WatchForParentExit() + if len(os.Args) < 2 { log.Fatal("Must provide server configuration") } diff --git a/lua/gitlab/server.lua b/lua/gitlab/server.lua index ff04766e..89cd22f8 100644 --- a/lua/gitlab/server.lua +++ b/lua/gitlab/server.lua @@ -95,6 +95,11 @@ M.start = function(callback) end local ok, obj = pcall(vim.system, { state.settings.server.binary, settings }, { + -- The `stdin` param opens a pipe on the server's stdin that nvim holds open but + -- never writes to. The server exits when it sees EOF on the pipe, which is how it + -- notices that nvim is gone in the cases the VimLeavePre autocmd cannot cover: + -- SIGKILL, an OOM kill, or a crash. + stdin = true, stdout = function(_, data) if data == nil or parsed_port ~= nil then return