add ListenWithID and StopListening#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds listener lifecycle management to the event package by introducing an ID-returning listener registration method and a way to deregister listeners, addressing use cases like cleaning up websocket listeners on disconnect.
Changes:
- Add
ListenWithIDto register listeners while returning an ID for later removal. - Add
StopListeningplus a newErrUnknownListenererror for unknown listener IDs. - Update examples to demonstrate listener deregistration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
event.go |
Switch listener storage to an ID-keyed map; add ListenWithID, StopListening, and ErrUnknownListener. |
examples_test.go |
Add a runnable example demonstrating unregistering a listener and verifying output stops. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type Event[T any] struct { | ||
| listeners []func(T) | ||
| listeners map[int]func(T) | ||
| mu sync.RWMutex | ||
| closed bool | ||
| next_id int | ||
| } |
There was a problem hiding this comment.
next_id uses snake_case. This is not idiomatic Go and is likely to be reported by stylecheck (ST1003). Rename to nextID (and update getID accordingly).
| // This ensures that triggering the event is thread-safe even if listeners are added or removed concurrently. | ||
| listeners := make([]func(T), len(e.listeners)) | ||
| copy(listeners, e.listeners) | ||
| var listeners []func(T) |
There was a problem hiding this comment.
This loop builds a slice via repeated append without preallocating capacity even though len(e.listeners) is known. With prealloc enabled, this may be reported and it also causes unnecessary allocations. Consider make([]func(T), 0, len(e.listeners)) before the loop.
| var listeners []func(T) | |
| listeners := make([]func(T), 0, len(e.listeners)) |
| func (e *Event[T]) Listen(f func(T)) error { | ||
| _, err := e.ListenWithID(f) | ||
| return err | ||
|
|
There was a problem hiding this comment.
There’s an extra blank line before the closing brace. With gofumpt/whitespace linters enabled, this formatting may fail linting. Please remove the blank line.
| func (e *Event[T]) ListenWithID(f func(T)) (int, error) { | ||
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
|
|
||
| if e.closed { | ||
| return ErrEventClosed | ||
| return -1, ErrEventClosed | ||
| } | ||
|
|
||
| e.listeners = append(e.listeners, f) | ||
| id := e.getID() | ||
|
|
||
| e.listeners[id] = f | ||
|
|
There was a problem hiding this comment.
ListenWithID will panic if e.listeners is nil (e.g., when Event is used as a zero value via var e event.Event[T] / new(Event[T])). Previously, the zero value worked because appending to a nil slice is safe. Consider lazily initializing the map under the lock when it’s nil to preserve zero-value usability and avoid panics.
| func (e *Event[T]) StopListening(id int) error { | ||
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
|
|
||
| _, ok := e.listeners[id] | ||
| if !ok { | ||
| return ErrUnknownListener | ||
| } | ||
| delete(e.listeners, id) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
StopListening is an exported method but it lacks a doc comment. stylecheck typically requires exported identifiers to have a comment that starts with the identifier name. Please add a // StopListening ... comment describing behavior (including what happens when the event is closed or the ID is unknown).
| } | ||
|
|
||
| func ExampleEvent_StopListening() { | ||
|
|
There was a problem hiding this comment.
There is a leading blank line inside the function body. This differs from the other examples in this file and may be flagged by gofumpt/whitespace linting. Please remove the initial blank line after the opening brace.
| triggerCount := 0 | ||
| listen_id, _ := exampleEvent.ListenWithID(func(v string) { | ||
| triggerCount++ | ||
| fmt.Printf("%d - %s\n", triggerCount, v) |
There was a problem hiding this comment.
listen_id uses snake_case, which is not idiomatic Go and may be flagged by stylecheck (ST1003) even in _test.go files. Rename to listenID for consistency with the rest of the examples.
| // ErrUnknownListener is returned when attempting to unregister an unknown id | ||
| var ErrUnknownListener = errors.New("listener id is unknown") |
There was a problem hiding this comment.
The exported error’s doc comment is missing a trailing period, which is likely to be flagged by stylecheck (ST1005) and is inconsistent with ErrEventClosed’s comment format. Please end the sentence with a period (and consider capitalizing “ID” for consistency in comments/messages).
| // ErrUnknownListener is returned when attempting to unregister an unknown id | |
| var ErrUnknownListener = errors.New("listener id is unknown") | |
| // ErrUnknownListener is returned when attempting to unregister an unknown ID. | |
| var ErrUnknownListener = errors.New("listener ID is unknown") |
|
Hi @jbert, I believe we could return the Don't worry about the Copilot comments, I will take a closer look at the repo in the next days and implement your change :) Thanks for contributing! |
|
Hi, I've updated the branch to address the copilot comments and your point about returning the ID from Listen. Thanks again :-) jb |
Hi,
Thanks very much for this library. I wanted to use it to push events out of a websocket, but also wanted to avoid leaking a listener (and any associated resources) if the websocket closed, so I added
ListenWithIDandStopListeningto allow unregistering a listener.I chose to avoid changing
Listento return an id, since that was a backwards-incompatible change.Thanks again.
regards,
jb