Skip to content

vendor: resenje.org/singleflight v0.4.3#48930

Merged
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:vendor-singleflight
Nov 22, 2024
Merged

vendor: resenje.org/singleflight v0.4.3#48930
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:vendor-singleflight

Conversation

@vvoland
Copy link
Copy Markdown
Contributor

@vvoland vvoland commented Nov 22, 2024 •
edited
Loading

full diff: janos/singleflight@v0.4.1...v0.4.3

Changes:

  • Fix incorrect Forget behavior
  • Make panic behavior consistent with x/sync package

@vvoland
vendor: resenje.org/singleflight v0.4.3
1551d95 
full diff: https://resenje.org/singleflight/compare/v0.4.1...v0.4.3

Changes:
- Fix incorrect `Forget` behavior
- Make panic behavior consistent with x/sync package

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland added this to the 28.0.0 milestone Nov 22, 2024
@vvoland vvoland self-assigned this Nov 22, 2024
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 22, 2024

oh! the diff link doesn't work with the vanity URL, so you need the one from github;

janos/singleflight@v0.4.1...v0.4.3

thaJeztah
thaJeztah reviewed Nov 22, 2024
View reviewed changes
Comment thread vendor/resenje.org/singleflight/singleflight.go
Comment on lines +123 to +125
if panicErr != nil {
panic(panicErr)
}
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Nov 22, 2024

Choose a reason for hiding this comment

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

Is there any situation where we could trip up a panic now, or would that already be such an exception that it already panicked through other ways, or a panic is the only right thing to do?

Copy link
Copy Markdown
Contributor Author

@vvoland vvoland Nov 22, 2024

Choose a reason for hiding this comment

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

The singleflight behavior for the same key should produce the same result, so it's very likely that the panic would happen again.

The x/sync/singleflight also does the same thing: https://cs.opensource.google/go/x/sync/+/refs/tags/v0.9.0:singleflight/singleflight.go;l=102

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 22, 2024

This one is definitely flaky now with runc 1.2.2; I think there were some patches in WIP branches that didn't get merged, and I'm trying to dig them up;

=== RUN   TestDiff
    diff_test.go:28: assertion failed: 
        --- expected
        +++ items
          []container.FilesystemChange(
        - 	{{Kind: s"A", Path: "/foo"}, {Kind: s"A", Path: "/foo/bar"}},
        + 	nil,
          )
        
--- FAIL: TestDiff (0.65s)

thaJeztah
thaJeztah approved these changes Nov 22, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 4def7cb into moby:master Nov 22, 2024
@thaJeztah thaJeztah mentioned this pull request Nov 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

@vvoland vvoland

Labels

area/dependencies process/cherry-picked status/2-code-review

Projects

None yet

Milestone

28.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@vvoland @thaJeztah