Skip to content

runtime: use runtime.AddCleanup in the standard library #70907

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.

Sign up for GitHub

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

Jump to bottom
Open
8 of 10 tasks
cagedmantis opened this issue Dec 18, 2024 · 18 comments
Open
8 of 10 tasks

runtime: use runtime.AddCleanup in the standard library #70907

cagedmantis opened this issue Dec 18, 2024 · 18 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.25

Comments

@cagedmantis
Copy link
Contributor

cagedmantis commented Dec 18, 2024 •
edited
Loading

AddCleanup has been added to the runtime (#67535). We should use runtime.AddCleanup instead of runtime.SetFinalizer in the standard library wherever it is possible.

Remaining instances of SetFinalizer (outside of runtime):

  • crypto/internal/boring: no longer supported. No use in updating.
  • crypto/tls: CL 664275
  • go/parser/testdata: test only change. Not worth modifying.
  • internal/types/testdata/check: test only change. Not worth modifying.
  • internal/poll: CL 671441
  • net: CL 638557 adds TODOs
  • os/exec (Current code relies on object resurrection)
  • reflect: test only change. Made easy modifications where possible CL 667595
  • syscall/js (Current code relies on object resurrection)
  • weak (intentionally used in test)

@mknyszek
@cagedmantis cagedmantis added NeedsFix The path to resolution is known, but the work has not been done. compiler/runtime Issues related to the Go compiler and/or runtime. labels Dec 18, 2024
@cagedmantis cagedmantis added this to the Go1.25 milestone Dec 18, 2024
@cagedmantis cagedmantis self-assigned this Dec 18, 2024
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638555 mentions this issue: os: use AddCleanup to close files

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638557 mentions this issue: net: use runtime.AddCleanup instead of runtime.SetFinalizer

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638556 mentions this issue: io: use runtime.AddCleanup instead of runtime.SetFinalizer

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638578 mentions this issue: os: simplify process status

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638576 mentions this issue: os: remove Process.mode field

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638579 mentions this issue: os: use AddCleanup, not SetFinalizer, for Process

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638575 mentions this issue: os: separate Process.handle into a separate memory allocation

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638577 mentions this issue: os: don't store reference count in Process.state

@cagedmantis cagedmantis mentioned this issue Jan 3, 2025
Closed
gopherbot pushed a commit that referenced this issue Feb 4, 2025
@ianlancetaylor @gopherbot
os: separate Process.handle into a separate memory allocation
5c2b5e0 
This is a step toward using AddCleanup rather than SetFinalizer
to close process handles.

For #70907

Change-Id: I7fb37461dd67b27135eab46fbdae94f0058ace85
Reviewed-on: https://go-review.googlesource.com/c/go/+/638575
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Griesemer <gri@google.com>
gopherbot pushed a commit that referenced this issue Feb 4, 2025
@ianlancetaylor @gopherbot
os: remove Process.mode field
b485e5b 
It's now redundant with checking whether the handle field is nil.

For #70907

Change-Id: I877f2a7c63d15ab5f8e3d2c9aa24776c2e3e2056
Reviewed-on: https://go-review.googlesource.com/c/go/+/638576
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
gopherbot pushed a commit that referenced this issue Feb 7, 2025
@ianlancetaylor @gopherbot
os: don't store reference count in Process.state
caf29da 
We only need a reference count in processHandle.

For #70907
Fixes #71564

Change-Id: I209ded869203dea10f12b070190774fb5f1d3d71
Reviewed-on: https://go-review.googlesource.com/c/go/+/638577
Commit-Queue: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
gopherbot pushed a commit that referenced this issue Feb 7, 2025
@ianlancetaylor @gopherbot
os: simplify process status
e7c9667 
Since it no longer holds a reference count, just use values.

For #70907

Change-Id: I19a42583988d4f8a9133b1c837356ca0179d688c
Reviewed-on: https://go-review.googlesource.com/c/go/+/638578
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
gopherbot pushed a commit that referenced this issue Feb 8, 2025
@ianlancetaylor @gopherbot
os: use AddCleanup, not SetFinalizer, for Process
bdbc5ca 
There is no reason to use a cleanup/finalizer for a Process that
doesn't use a handle, because Release doesn't change anything visible
about the process.

For #70907

Change-Id: I3b92809175523ceee2e07d601cc2a8e8b86321e0
Reviewed-on: https://go-review.googlesource.com/c/go/+/638579
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@neild neild mentioned this issue Feb 10, 2025
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/640735 mentions this issue: sync: use runtime.AddCleanup instead of runtime.SetFinalizer

gopherbot pushed a commit that referenced this issue Feb 13, 2025
@cagedmantis @gopherbot
os: use AddCleanup to close files
fdaac84 
This changes the finalizer mechanism used to close files from
runtime.SetFinalizer to runtime.AddCleanup.

Updates #70907

Change-Id: I47582b81b0ed69609dd9dac158ec7bb8819c8c77
Reviewed-on: https://go-review.googlesource.com/c/go/+/638555
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Feb 13, 2025
@cagedmantis @gopherbot
io: use runtime.AddCleanup instead of runtime.SetFinalizer
e4b12eb 
Replace the usage of runtime.SetFinalizer with runtime.AddCleanup.

Updates #70907

Change-Id: Id604ca44ea67dcf8f87797e27347c6f4e9ad0b86
Reviewed-on: https://go-review.googlesource.com/c/go/+/638556
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Carlos Amedee <carlos@golang.org>
TryBot-Bypass: Carlos Amedee <carlos@golang.org>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/640736 mentions this issue: cmd/go: use runtime.AddCleanup instead of runtime.SetFinalizer

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/648655 mentions this issue: net/http: use runtime.AddCleanup instead of runtime.SetFinalizer

gopherbot pushed a commit that referenced this issue Feb 14, 2025
@cagedmantis
sync: use runtime.AddCleanup instead of runtime.SetFinalizer
954e2c0 
This changes the use of finalizers to the cleanup implementation in
tests.

Updates #70907

Change-Id: I7d7289999a83fa53f538698f34294f7d9651c921
Reviewed-on: https://go-review.googlesource.com/c/go/+/640735
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Feb 14, 2025
@cagedmantis
cmd/go: use runtime.AddCleanup instead of runtime.SetFinalizer
a8487da 
Replace the usage of runtime.SetFinalizer with runtime.AddCleanup.
This changes a test and how when the Go command panics when a file is
left locked.

Updates #70907

Change-Id: I8d8c56d16486728f9bd4b910b81796ae506bda74
Reviewed-on: https://go-review.googlesource.com/c/go/+/640736
Reviewed-by: Sam Thanawalla <samthanawalla@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Feb 14, 2025
@cagedmantis
net/http: use runtime.AddCleanup instead of runtime.SetFinalizer
45447b4 
Replace the usage of runtime.SetFinalizer with runtime.AddCleanup in
tests.

Updates #70907

Change-Id: Idd3f1c07f6a7709352ca09948fbcb4a0ad9418bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/648655
Auto-Submit: Carlos Amedee <carlos@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/649459 mentions this issue: weak: test the use of runtime.AddCleanup

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/649458 mentions this issue: unique: use runtime.AddCleanup instead of runtime.SetFinalizer

gopherbot pushed a commit that referenced this issue Feb 24, 2025
@cagedmantis
unique: use runtime.AddCleanup instead of runtime.SetFinalizer
e382bf5 
Replace the usage of runtime.SetFinalizer with runtime.AddCleanup in
tests.

Updates #70907

Change-Id: I0d91b6af9643bde278215318f6176277373ddd19
Reviewed-on: https://go-review.googlesource.com/c/go/+/649458
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Feb 25, 2025
@cagedmantis
weak: test the use of runtime.AddCleanup
2f036e1 
This change adds a test case for runtime.AddCleanup.

Updates #70907

Change-Id: I29cba9dc5b40cec8e610215974e61ee47e10d00f
Reviewed-on: https://go-review.googlesource.com/c/go/+/649459
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/664275 mentions this issue: crypto/tls: use runtime.AddCleanup instead of runtime.SetFinalizer

@cagedmantis cagedmantis removed their assignment Apr 9, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/667555 mentions this issue: internal/poll: use runtime.AddCleanup instead of runtime.SetFinalizer

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/667595 mentions this issue: reflect: use runtime.AddCleanup instead of runtime.SetFinalizer

gopherbot pushed a commit that referenced this issue May 7, 2025
@cagedmantis @gopherbot
crypto/tls: use runtime.AddCleanup instead of runtime.SetFinalizer
17789bc 
Replace the usage of runtime.SetFinalizer with runtime.AddCleanup in
the certificate cache.

Updates #70907

Change-Id: Ieab6ff88dbc4083f11c1b475f11bd61521dbc638
Reviewed-on: https://go-review.googlesource.com/c/go/+/664275
Auto-Submit: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/671441 mentions this issue: internal/poll: use runtime.AddCleanup instead of runtime.SetFinalizer

gopherbot pushed a commit that referenced this issue May 13, 2025
@cagedmantis
reflect: use runtime.AddCleanup instead of runtime.SetFinalizer
b4e992b 
Replace a usage of runtime.SetFinalizer with runtime.AddCleanup in
the TestCallReturnsEmpty test. There is an additional use of
SetFinalizer in the reflect package which depends on object
resurrection and needs further refactoring to replace.

Updates #70907

Change-Id: I4c0e56c35745a225776bd611d026945efdaf96f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/667595
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue May 13, 2025
@cagedmantis
internal/poll: use runtime.AddCleanup instead of runtime.SetFinalizer
c44c4de 
Replace the use of SetFinalizer with AddCleanup.

For #70907

Change-Id: I0cb2c2985eb9285e5f92be9dbcb9d77acc0f59c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/671441
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Go1.25
Development

No branches or pull requests
3 participants
@cagedmantis @gopherbot @gabyhelp