Context support for GUI-driven clients and relays #1038
Conversation
abakum
commented
Dec 26, 2025
abakum
commented
There was a problem hiding this comment.
Pull request overview
This pull request adds context support for GUI-driven clients and relays, enabling graceful cancellation of long-running operations in the croc file transfer application. The implementation introduces context-aware versions of key functions and proper cleanup mechanisms.
Changes:
- Added context-aware file hashing functionality (
HashFileCtx) that can be canceled mid-operation - Implemented context-based TCP server management with graceful shutdown support
- Integrated context cancellation throughout the croc client lifecycle (Send/Receive operations)
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/ctx.go | New file implementing context-aware file hashing with cancellation support |
| src/utils/utils_test.go | Comprehensive tests for HashFileCtx covering cancellation scenarios |
| src/utils/utils.go | Minor fallback improvement for GetLocalIPs |
| src/tcp/ctx.go | Context management for TCP server with graceful shutdown |
| src/tcp/tcp.go | Refactored TCP server to use context-based lifecycle management |
| src/tcp/tcp_test.go | Tests for context-aware TCP server and various edge cases |
| src/croc/ctx.go | Context support for croc client operations |
| src/croc/croc.go | Integrated context cancellation throughout client lifecycle |
| src/croc/croc_test.go | Tests for context cancellation in various transfer scenarios |
Comments suppressed due to low confidence (1)
src/utils/ctx.go:1
- There's a race condition here. The context could be canceled between the select statement (line 46) and the ReadAt call (line 50), or between the ReadAt call and the context check (line 51). While this won't cause corruption, it creates inconsistent behavior. Consider using a non-blocking select after the read to check if cancellation occurred during the operation.
// ctx.go
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bar.Add(0) | ||
| } | ||
|
|
||
| b, err := imopartial.SumSectionReader(sr) |
There was a problem hiding this comment.
The variable imopartial is not defined in this file. It's defined in utils.go but not imported here. Either import it from utils.go by making it accessible, or define it locally in ctx.go.
| bar.Add(0) // Start the spinner | ||
| } | ||
|
|
||
| b, err := imofull.SumSectionReader(sr) |
There was a problem hiding this comment.
The variable imofull is not defined in this file. It's defined in utils.go but not imported here. Either import it from utils.go by making it accessible, or define it locally in ctx.go.
| // stopRoomCleanup chan struct{} | ||
| // replaced by stop ctx.go |
There was a problem hiding this comment.
Remove commented-out code rather than leaving it with a note. The version control system preserves this history.
| s.roomTTL = DEFAULT_ROOM_TTL | ||
| s.debugLevel = DEFAULT_LOG_LEVEL | ||
| s.stopRoomCleanup = make(chan struct{}) | ||
| // s.stopRoomCleanup = make(chan struct{}) replaced by stop |
There was a problem hiding this comment.
Remove commented-out code rather than leaving it with a note. The version control system preserves this history.
| // s.stopRoomCleanup = make(chan struct{}) replaced by stop |
| defer s.stop.wg.Done() | ||
| s.deleteOldRooms() | ||
| }() | ||
| // defer s.stopRoomDeletion() |
There was a problem hiding this comment.
Remove commented-out code rather than leaving it with a note. The version control system preserves this history.
| func (c *ctxFile) Seek(offset int64, whence int) (n int64, err error) { | ||
| select { | ||
| case <-c.ctx.Done(): | ||
| return 0, c.ctx.Err() | ||
| default: | ||
| n, err = c.f.Seek(offset, whence) | ||
| if c.ctx.Err() != nil { | ||
| return 0, c.ctx.Err() | ||
| } | ||
| return n, err | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a race condition here. The context could be canceled between the select statement (line 60) and the Seek call (line 64), or between the Seek call and the context check (line 65). While this won't cause corruption, it creates inconsistent behavior. Consider using a non-blocking select after the seek to check if cancellation occurred during the operation.
| go func() { | ||
| dc := &net.Dialer{ | ||
| Timeout: 100 * time.Millisecond, | ||
| } | ||
| if conn, err := dc.DialContext(s.stop.ctx, network, addr); err == nil { | ||
| log.Debugf("started TCP server on %s", addr) | ||
| conn.Close() | ||
| } else { | ||
| log.Errorf("started TCP server on %s : %v", addr, err) | ||
| s.stop.Cancel() | ||
| } | ||
| }() |
There was a problem hiding this comment.
This goroutine is fired-and-forgotten without any synchronization. If the main function returns before this goroutine completes, it may not execute fully. Consider adding this to the WaitGroup or using a synchronization mechanism to ensure proper cleanup.
| defer func() { | ||
| if r := recover(); r != nil { | ||
| if c.stop.gui { | ||
| log.Errorf("panic: %v", r) | ||
| c.stop.Cancel() | ||
| } else { | ||
| panic(r) | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
Suppressing panics in GUI mode can hide serious bugs. Consider logging the full stack trace with debug.Stack() to aid debugging, and potentially returning an error from this function instead of silently recovering.
| defer func() { | ||
| if r := recover(); r != nil { | ||
| if c.stop.gui { | ||
| log.Errorf("panic: %v", r) | ||
| c.stop.Cancel() | ||
| } else { | ||
| panic(r) | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
Suppressing panics in GUI mode can hide serious bugs. Consider logging the full stack trace with debug.Stack() to aid debugging, and potentially returning an error from this function instead of silently recovering.
| if r := recover(); r != nil { | ||
| if c.stop.gui { | ||
| log.Errorf("panic: %v", r) | ||
| c.stop.Cancel() | ||
| } else { | ||
| panic(r) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suppressing panics in GUI mode can hide serious bugs. Consider logging the full stack trace with debug.Stack() to aid debugging, and potentially returning an error from this function instead of silently recovering.
1 participant
