Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing defer close for RunWithTimeout #7

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

emar-kar
Copy link
Contributor

@emar-kar emar-kar commented Jun 7, 2024

No description provided.

Copy link
Owner

@WangYihang WangYihang left a comment

Choose a reason for hiding this comment

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

There might not be a need to close the done channel (Please correct me if I am wrong). When the function RunWithTimeout returns, either due to the completion of f() or a timeout signal, the done channel will no longer be in use. It's likely that the GoLang runtime will handle its deallocation appropriately.

As you can see in the following output, -gcflags="-m" will print optimization decisions, and done is not escaped to the heap.

$ go build -gcflags="-m" ./pkg/utils/timeout.go
# command-line-arguments
pkg/utils/timeout.go:14:5: can inline RunWithTimeout.func1
pkg/utils/timeout.go:9:55: inlining call to context.Background
pkg/utils/timeout.go:8:21: leaking param: f
pkg/utils/timeout.go:9:55: context.backgroundCtx{} escapes to heap
pkg/utils/timeout.go:14:5: func literal escapes to heap

But it seems more consistent with the pipeline design 1 and the Generator Pattern 2. I am more likely to accept this pull request. Thanks for your contribution.

Footnotes

  1. https://go.dev/blog/pipelines

  2. https://www.oreilly.com/library/view/concurrency-in-go/9781491941294/ch04.html

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.77%. Comparing base (e279609) to head (cbc8dc6).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
+ Coverage   24.68%   24.77%   +0.09%     
==========================================
  Files          26       26              
  Lines         786      787       +1     
==========================================
+ Hits          194      195       +1     
  Misses        575      575              
  Partials       17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WangYihang
Copy link
Owner

@emar-kar Any suggestions? Feel free to correct me if I am wrong, thanks a lot!

@emar-kar
Copy link
Contributor Author

emar-kar commented Jun 7, 2024

@WangYihang as far as I know, when you use make it always allocates memory on the heap. As soon as channel escapes the function, I would say it will be collected. But it leads to unpredictable time of collection, especially if there is data in the channel. I would prefer to have more clear understanding when resources are being released. If the tasks have huge consumption, it will require more work from GC. Maybe I just nitpicking, but I think all those things can be avoided, by small defer with closing the channel.

@WangYihang WangYihang merged commit 746577b into WangYihang:main Jun 8, 2024
1 check passed
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