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.

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

internal/fmtsort: channel tests make specious assumptions about object ordering #49431

Closed
randall77 opened this issue Nov 7, 2021 · 9 comments
Closed

internal/fmtsort: channel tests make specious assumptions about object ordering #49431

randall77 opened this issue Nov 7, 2021 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.22

Comments

@randall77
Copy link
Contributor

On the longtest builders:

--- FAIL: TestCompare (0.00s)
    sort_test.go:78: chan int: compare(0xc000196000,0xc00001e1e0)=1; expect -1
    sort_test.go:78: chan int: compare(0xc000196000,0xc00001e240)=1; expect -1
    sort_test.go:78: chan int: compare(0xc00001e1e0,0xc000196000)=-1; expect 1
    sort_test.go:78: chan int: compare(0xc00001e240,0xc000196000)=-1; expect 1
--- FAIL: TestOrder (0.00s)
    sort_test.go:227: map[chan int]string: got "CHAN1:1 CHAN2:2 CHAN0:0", want "CHAN0:0 CHAN1:1 CHAN2:2"
FAIL
FAIL	internal/fmtsort	0.004s

I think this is happening because chans[0] is in pointer space higher than chans[1] and chans[2]. It seems that these tests expect channels to sort in the order they are allocated, which isn't necessarily true.

@robpike
@randall77 randall77 added this to the Go1.18 milestone Nov 7, 2021
@robpike
Copy link
Contributor

robpike commented Nov 8, 2021

Yes, it looks like that's what's happening. Not sure how to make the test robust without essentially duplicating the package's own logic, which is unfortunate. Ideas welcome.

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 8, 2021
@gopherbot
Copy link

Change https://golang.org/cl/361918 mentions this issue: internal/fmtsort: order channels in test in memory address order

@bcmills
Copy link
Contributor

bcmills commented Nov 8, 2021 •
edited

It seems to me that sorting dynamically-allocated pointers by pointer value is inherently nondeterministic, given Go's memory model: if the values are relocated during the test, they will end up out of order. (That is: this test, strictly speaking, requires #46787.)

The fact that the language provides no way to statically allocate channel values further complicates the matter (compare #28366).

Without a pinning API, I think this test fundamentally cannot be written in a way is guaranteed by the language spec to be deterministic.

@randall77
Copy link
Contributor Author

Reopening to make this issue about making this test more robust.

@randall77 randall77 reopened this Nov 8, 2021
@randall77 randall77 modified the milestones: Go1.18, Unplanned Nov 8, 2021
@randall77 randall77 changed the title internal/fmtsort: channel ordering not deterministic internal/fmtsort: channel tests make specious assumptions about object ordering Nov 8, 2021
@bcmills bcmills mentioned this issue Nov 22, 2021
Closed
@golang golang deleted a comment from gopherbot Nov 30, 2021
@rsc
Copy link
Contributor

rsc commented Nov 30, 2021

@randall77, you reopened this bug. What is it meant to track now? Use of a Pinner?
It should be a long time before we have objects moving in the heap, and channels are pretty much always going to be in the heap (especially channels held in globals).

@randall77
Copy link
Contributor Author

Yes, just to make the test (and the actual code, maybe?) less dependent on object addresses. I'm not entirely clear on how that would happen, exactly. Maybe there's nothing concrete to do here.

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Sep 16, 2023
@qiulaidongfeng
internal/fmtsort: makeChans pin pointer
1f70fa3 
Complete TODO.

For golang#49431

Change-Id: Ifc79741ad44cd9631be49d4674db0ee52bbf6486
@qiulaidongfeng qiulaidongfeng mentioned this issue Sep 16, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/528796 mentions this issue: internal/fmtsort: makeChans pin pointer

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Oct 3, 2023
@qiulaidongfeng
internal/fmtsort: makeChans pin pointer
cfe4ed2 
Complete TODO.

For golang#49431

Change-Id: Ifc79741ad44cd9631be49d4674db0ee52bbf6486
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Oct 26, 2023
@qiulaidongfeng
internal/fmtsort: makeChans pin pointer
9f52aaa 
Complete TODO.

For golang#49431

Change-Id: Ifc79741ad44cd9631be49d4674db0ee52bbf6486
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Oct 27, 2023
@qiulaidongfeng
internal/fmtsort: makeChans pin pointer
52284ba 
Complete TODO.

For golang#49431

Change-Id: Ifc79741ad44cd9631be49d4674db0ee52bbf6486
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Oct 28, 2023
@qiulaidongfeng
internal/fmtsort: makeChans pin pointer
02cdea7 
Complete TODO.

For golang#49431

Change-Id: Ifc79741ad44cd9631be49d4674db0ee52bbf6486
gopherbot pushed a commit that referenced this issue Oct 30, 2023
@qiulaidongfeng @gopherbot
internal/fmtsort: makeChans pin pointer
9c2ab20 
Complete TODO.

For #49431

Change-Id: I1399205e430ebd83182c3e0c4becf1fde32d433e
GitHub-Last-Rev: 02cdea7
GitHub-Pull-Request: #62673
Reviewed-on: https://go-review.googlesource.com/c/go/+/528796
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Keith Randall <khr@golang.org>
Commit-Queue: Keith Randall <khr@golang.org>
Run-TryBot: qiulaidongfeng <2645477756@qq.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
@bcmills
Copy link
Contributor

bcmills commented Oct 30, 2023

@qiulaidongfeng: is this fixed as of https://go.dev/cl/528796, or is there more to do?

@bcmills bcmills modified the milestones: Unplanned, Go1.22 Oct 30, 2023
@randall77
Copy link
Contributor Author

I'm going to close, I think this is done.
Maybe someday someone can figure out how to write this test that doesn't depend on pinning. But that doesn't need an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
6 participants
@rsc @robpike @bcmills @randall77 @gopherbot @thanm