Skip to content

Conversation

@hwbrzzl
Copy link
Contributor

@hwbrzzl hwbrzzl commented Oct 20, 2025

πŸ“‘ Description

Related to:

urfave/cli#1242
urfave/cli#2176

Previous:

image

Current:

image

βœ… Checks

  • Added test cases for my code

Copilot AI review requested due to automatic review settings October 20, 2025 13:09
@hwbrzzl hwbrzzl requested a review from a team as a code owner October 20, 2025 13:09
Copy link
Contributor

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

This PR fixes an issue where CLI commands could not be run concurrently by refactoring the application instance from a shared global object to a new instance created per command execution.

  • Refactored Application struct to store command metadata instead of maintaining a shared cli.Command instance
  • Added a new instance() method that creates a fresh cli.Command for each execution
  • Added comprehensive concurrent test coverage to verify the fix

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
console/application.go Core fix - replaced shared instance with per-execution instance creation
console/application_test.go Added concurrent test with atomic counters to verify thread safety
console/cli_helper_test.go Fixed test to create isolated application instance per test run
Comments suppressed due to low confidence (1)

console/application_test.go:172

  • This increment operation is not thread-safe. Should use atomic.AddInt64(&testCommand, 1) to prevent race conditions during concurrent test execution.
	testCommand++

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite Current: ddd9626 Previous: 32ff364 Ratio
Benchmark_DecryptString 6278 ns/op 2032 B/op 16 allocs/op 2055 ns/op 2032 B/op 16 allocs/op 3.05
Benchmark_DecryptString - ns/op 6278 ns/op 2055 ns/op 3.05
BenchmarkFile_ReadWrite 317578 ns/op 6258 B/op 99 allocs/op 169137 ns/op 6258 B/op 99 allocs/op 1.88
BenchmarkFile_ReadWrite - ns/op 317578 ns/op 169137 ns/op 1.88

This comment was automatically generated by workflow using github-action-benchmark.

@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

❌ Patch coverage is 68.75000% with 20 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (v1.16.x@7264b71). Learn more about missing BASE report.

Files with missing lines Patch % Lines
console/console/help_command.go 0.00% 10 Missing ⚠️
console/application.go 83.67% 4 Missing and 4 partials ⚠️
console/service_provider.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             v1.16.x    #1243   +/-   ##
==========================================
  Coverage           ?   68.00%           
==========================================
  Files              ?      216           
  Lines              ?    11605           
  Branches           ?        0           
==========================================
  Hits               ?     7892           
  Misses             ?     3333           
  Partials           ?      380           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@devhaozi
Copy link
Member

image

urfave/cli#2176

@hwbrzzl
Copy link
Contributor Author

hwbrzzl commented Oct 20, 2025

em.. local passed, strange, will check it.

almas-x
almas-x previously approved these changes Oct 20, 2025
Copy link
Contributor

@almas-x almas-x left a comment

Choose a reason for hiding this comment

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

LGTMπŸ‘

cliFlags = append(cliFlags, helpFlag)
}

cliFlags = append(cliFlags, noANSIFlag)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Image

--version should not be shown in the image above. Hence, I think it's better to hide the global options directly. Users only need to know the options.

Image

@hwbrzzl
Copy link
Contributor Author

hwbrzzl commented Oct 25, 2025

Please take a look again @goravel/core-developers

Copy link
Member

@kkumar-gcc kkumar-gcc left a comment

Choose a reason for hiding this comment

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

LGTM πŸ‘

@hwbrzzl hwbrzzl merged commit 2b42a80 into v1.16.x Oct 25, 2025
16 of 17 checks passed
@hwbrzzl hwbrzzl deleted the bowen/fix-command-concurrent branch October 25, 2025 09:16
@oprudkyi
Copy link
Contributor

Hi @hwbrzzl
I have added fix into urfave/cli urfave/cli#2215
additionally to HelpFlag (shut by HideHelp) there are also issues with slices and maps

@hwbrzzl
Copy link
Contributor Author

hwbrzzl commented Oct 25, 2025

@oprudkyi Thanks, amazing, looking forward to merging it.

hwbrzzl added a commit that referenced this pull request Oct 26, 2025
* correctly set cc and bcc headers (#1144)

* correct the error return from SendMailJob handle (#1147)

* fix: [#743] package make command generates correct code (#1151) (#1152)

(cherry picked from commit 93dc8a3)

* fix: [#749] The path is incorrect when publishing package files (#1157)

* chore: optimize assertions for package installation (#1160)

* fix: The configuredServiceProviders, publishes and publishGroups are not reset when Booting (#1162)

* fix: The configuredServiceProviders, publishes and publishGroups are not reset when Booting

* optimize

* fix: [#738] The Orm Creating event can be triggered when the query with the Model method (#1166)

* fix: [#738] The Orm Creating event can be triggered when the query with the Model method

* fix tests

* upgrade: v1.16.1

* fix: [#762] handle panic when using transaction (#1183)

* fix: [#762] handle panic when using transaction

* v1.16.2

* optimize

* fix lint

* fix: [#768] facades.DB will panic when migrating a new column (#1185)

* fix: [#768] facades.DB will panic when migrating a new column

* optimize

* optimize

* feat: [#770] Add a SelectRaw function for the ORM (#1186)

* fix: [#770] Add a SelectRaw function for the ORM

* fix: [#770] Add a SelectRaw function for the ORM

* fix ci

* upgrade: v1.16.3

* fix: comand cannot be run concurrently (#1243)

* fix: comand cannot be run concurrently

* fix ci

* fix ci

* optimize

* optimize

* optimize

* optimize

* optimize global options

* fix ci

* upgrade v1.16.4

* optimize

* optimize

---------

Co-authored-by: krishan kumar <84431594+kkumar-gcc@users.noreply.github.com>
Co-authored-by: ALMAS <almas.cc@icloud.com>
hwbrzzl added a commit that referenced this pull request Oct 31, 2025
* correctly set cc and bcc headers (#1144)

* correct the error return from SendMailJob handle (#1147)

* fix: [#743] package make command generates correct code (#1151) (#1152)

(cherry picked from commit 93dc8a3)

* fix: [#749] The path is incorrect when publishing package files (#1157)

* chore: optimize assertions for package installation (#1160)

* fix: The configuredServiceProviders, publishes and publishGroups are not reset when Booting (#1162)

* fix: The configuredServiceProviders, publishes and publishGroups are not reset when Booting

* optimize

* fix: [#738] The Orm Creating event can be triggered when the query with the Model method (#1166)

* fix: [#738] The Orm Creating event can be triggered when the query with the Model method

* fix tests

* upgrade: v1.16.1

* fix: [#762] handle panic when using transaction (#1183)

* fix: [#762] handle panic when using transaction

* v1.16.2

* optimize

* fix lint

* fix: [#768] facades.DB will panic when migrating a new column (#1185)

* fix: [#768] facades.DB will panic when migrating a new column

* optimize

* optimize

* feat: [#770] Add a SelectRaw function for the ORM (#1186)

* fix: [#770] Add a SelectRaw function for the ORM

* fix: [#770] Add a SelectRaw function for the ORM

* fix ci

* upgrade: v1.16.3

* fix: comand cannot be run concurrently (#1243)

* fix: comand cannot be run concurrently

* fix ci

* fix ci

* optimize

* optimize

* optimize

* optimize

* optimize global options

* fix ci

* upgrade v1.16.4

* fix: [#807] queue.Shutdown doesn't stop the queue as expected (#1252)

* fix: [#807] queue.Shutdown doesn't stop the queue as expected

* optimize

* upgrade: v1.16.5

* fix

* Update queue/worker_test.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* optimize

---------

Co-authored-by: krishan kumar <84431594+kkumar-gcc@users.noreply.github.com>
Co-authored-by: ALMAS <almas.cc@icloud.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
hwbrzzl added a commit that referenced this pull request Oct 31, 2025
* correctly set cc and bcc headers (#1144)

* correct the error return from SendMailJob handle (#1147)

* fix: [#743] package make command generates correct code (#1151) (#1152)

(cherry picked from commit 93dc8a3)

* fix: [#749] The path is incorrect when publishing package files (#1157)

* chore: optimize assertions for package installation (#1160)

* fix: The configuredServiceProviders, publishes and publishGroups are not reset when Booting (#1162)

* fix: The configuredServiceProviders, publishes and publishGroups are not reset when Booting

* optimize

* fix: [#738] The Orm Creating event can be triggered when the query with the Model method (#1166)

* fix: [#738] The Orm Creating event can be triggered when the query with the Model method

* fix tests

* upgrade: v1.16.1

* fix: [#762] handle panic when using transaction (#1183)

* fix: [#762] handle panic when using transaction

* v1.16.2

* optimize

* fix lint

* fix: [#768] facades.DB will panic when migrating a new column (#1185)

* fix: [#768] facades.DB will panic when migrating a new column

* optimize

* optimize

* feat: [#770] Add a SelectRaw function for the ORM (#1186)

* fix: [#770] Add a SelectRaw function for the ORM

* fix: [#770] Add a SelectRaw function for the ORM

* fix ci

* upgrade: v1.16.3

* fix: comand cannot be run concurrently (#1243)

* fix: comand cannot be run concurrently

* fix ci

* fix ci

* optimize

* optimize

* optimize

* optimize

* optimize global options

* fix ci

* upgrade v1.16.4

* fix: [#807] queue.Shutdown doesn't stop the queue as expected (#1252)

* fix: [#807] queue.Shutdown doesn't stop the queue as expected

* optimize

* upgrade: v1.16.5

* fix

* Update queue/worker_test.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* optimize

---------

Co-authored-by: krishan kumar <84431594+kkumar-gcc@users.noreply.github.com>
Co-authored-by: ALMAS <almas.cc@icloud.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants