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.

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

Sonic: nested slices references don't work on arm64 #1811

Open
gbjk opened this issue Feb 24, 2025 · 5 comments
Open

Sonic: nested slices references don't work on arm64 #1811

gbjk opened this issue Feb 24, 2025 · 5 comments
Assignees
Labels

Comments

@gbjk
Copy link
Collaborator

gbjk commented Feb 24, 2025

With the addition of sonic as the default json decoder, we've found that okx does not unmarshal responses correctly.
Specifically, okx's SendHTTPRequest accepts any which should contain a pointer to something.
It then tries to wrap that in another slice of any in most situations, depending on the useAsItIs flag and the type of the data.
The issue is that sonic replaces the pointer inside the top slice, and so the caller's pointer is never changed.
The cleanest way to reproduce this I've found so far is:

func TestSonic(t *testing.T) {
	a := []string{}
	b := []any{&a}
	if err := sonic.ConfigStd.Unmarshal([]byte(`[["one","2"]]`), &b); err != nil {
		t.Fatalf("Unmarshal must not error: %s", err)
	}
	if _, ok := b[0].(*[]string); !ok {
		t.Fatalf("b[0] must still a *[]string; Got: %T", b[0])
	}
	if b[0] != &a {
		t.Error("b[0] must still be *a")
	}
}

Giving:

GOARCH=arm64 go test ./exchanges/okx/... -run TestSonic
--- FAIL: TestSonic (0.00s)
    okx_test.go:598: b[0] must still a *[]string; Got: []interface {}

versus:

GOARCH=amd64 go test ./exchanges/okx/... -run TestSonic
--- PASS: TestSonic (0.01s)

Which yields an interesting difference on arm64, in that it can't even assert *[]string.
We've considered sidestepping this by changing okx's behaviour, but it's non-trivial and dirty.
For now #1794 is merged to turn sonic off for arm64 until this is fixed.

This issue is to track the solution in sonic, and any other possible mitigations we might come up with in the meantime.

@gbjk gbjk added the bug label Feb 24, 2025
@gbjk gbjk self-assigned this Feb 24, 2025
@gbjk
Copy link
Collaborator Author

gbjk commented Feb 24, 2025

Updated test to remove more superfluous distractions.
This happens on any [] any that contains pointers, and doesn't need an object wrapper.
Also stripped out assert, so we can use-as is with sonic team.

@gbjk
Copy link
Collaborator Author

gbjk commented Feb 24, 2025

Raised bytedance/sonic#750

@thrasher- thrasher- moved this to In progress in GoCryptoTrader Kanban Feb 24, 2025
@gbjk
Copy link
Collaborator Author

gbjk commented Feb 27, 2025

Raised bytedance/sonic#758

@gbjk
Copy link
Collaborator Author

gbjk commented Feb 27, 2025

Raised bytedance/sonic#750

First bug confirmed fixed on current (unreleased) main.

@gbjk
Copy link
Collaborator Author

gbjk commented Feb 28, 2025

Raised bytedance/sonic#758

Second bug confirmed fixed on unreleased main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In progress
Development

No branches or pull requests

1 participant