Conversation
|
Hi @SebTardif. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Please take note of https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#ai-guidance /ok-to-test |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 23 files with indirect coverage changes @@ Coverage Diff @@
## main #21766 +/- ##
==========================================
- Coverage 73.03% 73.00% -0.04%
==========================================
Files 448 448
Lines 31579 31583 +4
==========================================
- Hits 23065 23057 -8
- Misses 8511 8523 +12
Partials 3 3 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/retest pull-etcd-e2e-amd64 |
|
/retest-required |
|
#21779 added a test for filelock leak in |
|
ping @ahrtr for review |
| @@ -174,6 +180,7 @@ func Create(lg *zap.Logger, dirpath string, metadata []byte) (*WAL, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| closeF = false // renameWAL takes ownership of f via w | |||
There was a problem hiding this comment.
should we move this line after the defer statement (line 196)? otherwise, the file won't be closed via w.cleanupWAL
There was a problem hiding this comment.
Good catch. Moved closeF=false to after the cleanupWAL defer registration in c4038b12c. Now if renameWAL fails, our defer still closes f directly. Once the cleanupWAL defer is in place, it takes over and we disable our defer.
| @@ -97,7 +97,7 @@ type WAL struct { | |||
| // Create creates a WAL ready for appending records. The given metadata is | |||
| // recorded at the head of each WAL file, and can be retrieved with ReadAll | |||
| // after the file is Open. | |||
| func Create(lg *zap.Logger, dirpath string, metadata []byte) (*WAL, error) { | |||
| func Create(lg *zap.Logger, dirpath string, metadata []byte) (_ *WAL, err error) { | |||
There was a problem hiding this comment.
why not just using retErr error in return? it's easy to know it's return error and it won't be conflicted with errors defined in the function.
and I don't think we need this closeF := true. The f.Close() is allowed to close twice.
|
/retest-required |
| @@ -135,6 +135,12 @@ func Create(lg *zap.Logger, dirpath string, metadata []byte) (*WAL, error) { | |||
| ) | |||
| return nil, err | |||
| } | |||
| closeF := true | |||
There was a problem hiding this comment.
pls just remove this closeF, see https://github.com/etcd-io/etcd/pull/21766/changes#r3281700617
c4038b1 to
b18b3bc
Compare
|
Addressed review feedback: removed |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, SebTardif, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cc @fuweid |
|
@SebTardif Could you please do a rebase? Thanks! I have added this PR to the agenda for the next triage meeting. https://docs.google.com/document/d/16XEGyPBisZvmmoIHSZzv__LoyOeluC5a4x353CX0SIM/edit?tab=t.xjc2zly8zbof |
On Open, close the WAL if OpenDir fails so segment locks and the filePipeline goroutine are not leaked. On Create, use a named return so a deferred Close covers Seek, Preallocate, encode, and snapshot errors on the locked file. purgeFile already closes the flock on os.Remove failure on main. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
SebTardif
changed the title
b18b3bc to
702fd61
Compare
Yes. Rebased onto current |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, SebTardif, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
Fix resource leaks in WAL Open and Create error handling.
fileutil.purgeFilealready closes the flock whenos.Removefails on currentmain, so that hunk is no longer in this PR.1.
wal.Open: FD + goroutine leak whenOpenDirfailsCall chain:
Open→openAtIndex(succeeds, allocating file locks inw.locks, afilePipelinegoroutine inw.fp, and a pre-allocated temp file) →fileutil.OpenDir(w.dir)(fails)Trigger: Any filesystem error on the WAL directory between
openAtIndexcompleting andOpenDir(e.g., directory removed by concurrent process, permission change, or FD exhaustion).Leak:
w.locks(locked WAL segment FDs),w.fp(goroutine + pre-allocated temp file) are never released. Thereturn nil, errdiscardswwithout callingw.Close().Fix: Call
w.Close()before returning the error.2.
wal.Create: Locked FD leak on early error pathsCall chain:
Create→createNewWALFile(succeeds, returns locked filef) →f.Seek/Preallocate/newFileEncoder/saveCrc/encode/SaveSnapshot(any fails)Trigger: Disk full during Preallocate, I/O error during seek, or encoding failure.
Leak: The locked file
fis never closed. Whiledefer os.RemoveAll(tmpdirpath)removes the file from disk, the FD and advisory lock remain held.Fix: Use a named return and a deferred closure that calls
f.Close()when returning an error. Duplicate close afterrenameWALtakes ownership is harmless peros.File.Close.