Skip to content

Conversation

@songzhibin97
Copy link
Contributor

No description provided.

@songzhibin97 songzhibin97 requested a review from gballet as a code owner April 21, 2024 01:34
ks.mu.Lock()
defer ks.mu.Unlock()
if unl, found := ks.unlocked[addr]; found {
ks.mu.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the semantics. Previously, it released the lock before invoking ks.expire, but with your changes it retains the lock during the call.

Ostensibly, this was coded like it is for a reason. Please motivate why you consider this change safe/needed

Copy link
Contributor

Choose a reason for hiding this comment

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

If you just want to remove some code, but retain the semantics, you could do

    ks.mu.Lock() 
    unl, found := ks.unlocked[addr]
    ks.mu.Unlock()
    if found{
        ks.expire(addr, unl, time.Duration(0)*time.Nanosecond)
    }
    return nil

if err != nil {
t.Fatalf("error opening era file: %v", err)
}
defer f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit non-obvious,but era.From takes an io.ReadSeekCloser, and era.Close closes the f. So this change would make it a double-close

@holiman
Copy link
Contributor

holiman commented Apr 23, 2024

Please make a habit of always writing PR descriptions

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman changed the title fix: open file used up but not closed accounts, cmd/geth, core: close opened files Apr 24, 2024
@holiman holiman added this to the 1.14.1 milestone Apr 24, 2024
@holiman holiman merged commit f8820f1 into ethereum:master Apr 30, 2024
jorgemmsilva pushed a commit to iotaledger/go-ethereum that referenced this pull request Jun 17, 2024
* fix: open file used up but not closed

* feat: more same case

* feat: accept conversation
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 24, 2025
* fix: open file used up but not closed

* feat: more same case

* feat: accept conversation
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.

2 participants