[PR #1598] [MERGED] Fix panic in handleSubscribeHTTP when client disconnects during publish #1656

New issue

Closed

opened 2026-05-07 01:03:06 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-07 01:03:06 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/binwiederhier/ntfy/pull/1598
Author: @binwiederhier
Created: 2/8/2026
Status: ✅ Merged
Merged: 2/8/2026
Merged by: @binwiederhier

Base: main ← Head: crashfix

📝 Commits (1)

3647d39 Fix panic in handleSubscribeHTTP when client disconnects during publish

📊 Changes

1 file changed (+11 additions, -5 deletions)

View changed files

📝 server/server.go (+11 -5)

📄 Description

(This PR is AI generated, but a human [me] has verified that it is accurate)

Summary

Fixes a server crash (nil pointer dereference / SIGSEGV) in handleSubscribeHTTP caused by a race between a topic.Publish goroutine flushing the HTTP response writer and the handler returning after client disconnect.
Replaces the existing wlock.TryLock() hack with a proper wlock.Lock() + closed flag pattern that correctly waits for in-flight writes to finish and prevents future writes to a cleaned-up response writer.

Root cause

topic.Publish() copies the subscribers map and calls each subscriber in its own goroutine. When a client disconnects:

handleSubscribeHTTP returns, triggering deferred cleanup
Go's HTTP server cleans up the response writer
But a Publish goroutine may still be in-flight, calling Flush() on the now-invalid writer

The old TryLock() approach had two failure modes:

TryLock succeeds (no sub running): future sub() calls deadlock on wlock.Lock() (goroutine leak, since the lock is never released)
TryLock fails (sub is running): the handler returns immediately without waiting, and Go cleans up the writer while sub is still flushing — exactly the panic

Fix

The new approach:

Deferred cleanup uses a blocking wlock.Lock() to wait for any in-flight sub() call to finish, then sets closed = true
sub() function checks the closed flag after acquiring the lock — if the connection has been closed, it returns without touching the response writer

This guarantees the response writer is never accessed after the handler returns.

Stack trace this fixes

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x5b3525]

goroutine 4355370 [running]:
bufio.(*Writer).Flush(0xc061282940)
net/http.(*response).FlushError(0xc0329b7ce0)
net/http.(*response).Flush(0xc04935ba10?)
heckel.io/ntfy/v2/server.(*Server).handleSubscribeHTTP.func2(...)
        server/server.go:1454
heckel.io/ntfy/v2/server.(*topic).Publish.func1.1(...)
        server/topic.go:117

Test plan

go vet ./server/ passes
Verify with make check
Soak test under load with concurrent publishers and disconnecting subscribers

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/binwiederhier/ntfy/pull/1598 **Author:** [@binwiederhier](https://github.com/binwiederhier) **Created:** 2/8/2026 **Status:** ✅ Merged **Merged:** 2/8/2026 **Merged by:** [@binwiederhier](https://github.com/binwiederhier) **Base:** `main` ← **Head:** `crashfix` --- ### 📝 Commits (1) - [`3647d39`](https://github.com/binwiederhier/ntfy/commit/3647d3975cc300a86ba8a2fc28efe3235c0ffd9b) Fix panic in handleSubscribeHTTP when client disconnects during publish ### 📊 Changes **1 file changed** (+11 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `server/server.go` (+11 -5) </details> ### 📄 Description (This PR is AI generated, but a human [me] has verified that it is accurate) ## Summary - **Fixes a server crash** (nil pointer dereference / SIGSEGV) in `handleSubscribeHTTP` caused by a race between a `topic.Publish` goroutine flushing the HTTP response writer and the handler returning after client disconnect. - Replaces the existing `wlock.TryLock()` hack with a proper `wlock.Lock()` + `closed` flag pattern that correctly waits for in-flight writes to finish and prevents future writes to a cleaned-up response writer. ## Root cause `topic.Publish()` copies the subscribers map and calls each subscriber in its own goroutine. When a client disconnects: 1. `handleSubscribeHTTP` returns, triggering deferred cleanup 2. Go's HTTP server cleans up the response writer 3. But a `Publish` goroutine may still be in-flight, calling `Flush()` on the now-invalid writer The old `TryLock()` approach had two failure modes: - **TryLock succeeds** (no `sub` running): future `sub()` calls deadlock on `wlock.Lock()` (goroutine leak, since the lock is never released) - **TryLock fails** (`sub` is running): the handler returns immediately without waiting, and Go cleans up the writer while `sub` is still flushing — exactly the panic ## Fix The new approach: - **Deferred cleanup** uses a blocking `wlock.Lock()` to wait for any in-flight `sub()` call to finish, then sets `closed = true` - **`sub()` function** checks the `closed` flag after acquiring the lock — if the connection has been closed, it returns without touching the response writer This guarantees the response writer is never accessed after the handler returns. ## Stack trace this fixes ``` panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x5b3525] goroutine 4355370 [running]: bufio.(*Writer).Flush(0xc061282940) net/http.(*response).FlushError(0xc0329b7ce0) net/http.(*response).Flush(0xc04935ba10?) heckel.io/ntfy/v2/server.(*Server).handleSubscribeHTTP.func2(...) server/server.go:1454 heckel.io/ntfy/v2/server.(*topic).Publish.func1.1(...) server/topic.go:117 ``` ## Test plan - [x] `go vet ./server/` passes - [ ] Verify with `make check` - [ ] Soak test under load with concurrent publishers and disconnecting subscribers --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>