Skip to content

Conversation

@ovaistariq
Copy link
Contributor

Summary

  • Replaced custom semaphore implementation marked with TODO with standard golang.org/x/sync/semaphore package
  • Updated test files to use the new NewSemaphore() constructor
  • Added proper error handling for semaphore acquire operations

Changes Made

  1. Removed deprecated code (core/utils.go):

    • Removed custom semaphore type based on channels
    • Removed empty struct that was only used by the semaphore
    • Eliminated TODO comment about replacing with standard library
  2. Implemented modern semaphore (core/utils.go):

    • Created Semaphore struct wrapping golang.org/x/sync/semaphore.Weighted
    • Added NewSemaphore() constructor
    • Maintained same P() and V() API for backward compatibility
    • Added error handling for Acquire() calls (panics on error to maintain existing behavior)
  3. Updated test usage (core/goofys_common_test.go):

    • Replaced make(semaphore, n) with NewSemaphore(n)
    • No other changes needed due to maintained API compatibility

Test Plan

  • Code compiles successfully (go build)
  • Static analysis passes (go vet)
  • Linter passes (golangci-lint)
  • Code formatting is correct (go fmt)
  • Full test suite passes (requires cloud backend configuration)

This change eliminates technical debt and improves code maintainability by using the standard semaphore implementation.

🤖 Generated with Claude Code

- Remove custom semaphore implementation marked with TODO
- Replace with standard golang.org/x/sync/semaphore package
- Update test files to use NewSemaphore constructor
- Add proper error handling for Acquire operations
- Maintain same API for backward compatibility

This eliminates technical debt and improves code maintainability.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Copilot AI review requested due to automatic review settings July 16, 2025 18:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Refactors the internal semaphore to use golang.org/x/sync/semaphore and updates tests to use the new constructor

  • Removes custom channel-based semaphore and related types
  • Introduces Semaphore wrapper around semaphore.Weighted with NewSemaphore, P, and V
  • Updates test instantiations from make(semaphore, n) to NewSemaphore(n)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
core/utils.go Removed old semaphore type and added Semaphore wrapper with Acquire/Release
core/goofys_common_test.go Replaced make(semaphore, ...) calls with NewSemaphore(...)
Comments suppressed due to low confidence (1)

core/utils.go:180

  • Exported functions and types should have GoDoc comments. Please add a brief comment above NewSemaphore, Semaphore, P, and V explaining their purpose and behavior.
func NewSemaphore(n int64) *Semaphore {

ovaistariq and others added 3 commits July 16, 2025 14:39
- Add comprehensive GoDoc comments for Semaphore type and methods
- Change NewSemaphore parameter from int64 to int for API consistency
- Document the P (wait) and V (signal) operations

Addresses PR review feedback from Copilot.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
The test was incorrectly trying to release semaphore slots that were
already in use by goroutines, causing "released more than held" panic
with the new golang.org/x/sync/semaphore implementation.

Changed the pattern to properly wait for all goroutines to complete by
acquiring all slots back instead of trying to release them again.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
The previous fix was incomplete. The old channel-based semaphore had
different semantics - P() would send to channel and V() would receive.
The new semaphore properly tracks acquire/release counts.

Fixed the pattern to:
1. Acquire slot before spawning each goroutine (limits concurrency)
2. Release slot when goroutine completes (via defer)
3. Wait for completion by acquiring all slots, then release them

This prevents "released more than held" panics when there are more
items to process than the concurrency limit.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@ovaistariq ovaistariq merged commit 4e48555 into main Jul 16, 2025
5 checks passed
@ovaistariq ovaistariq deleted the feature/dead-code-analysis branch July 16, 2025 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants