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

Out of memory error on comparing protobuf types #930

Open
alexus1024 opened this issue Apr 21, 2020 · 6 comments
Open

Out of memory error on comparing protobuf types #930

alexus1024 opened this issue Apr 21, 2020 · 6 comments
Labels
Projects
Milestone

Comments

@alexus1024
Copy link

alexus1024 commented Apr 21, 2020

Currently, attempt to compare protobuf-generated types (from *.protoc files) leads to hang or
fatal error: runtime: out of memory

Maybe, it can be solved by respecting pragma.DoNotCompare during executing assert.Equal and similar requests.

It happens at
File: ../../github.com/stretchr/testify/assert/assertions.go
1567: func diff(expected interface{}, actual interface{}) string ..
and then call for spewConfig.Sdump()

Related issue:
davecgh/go-spew#66

Also, setting spew.ConfigState.MaxDepth to reasonable value helps prevent hanging.

rolinh added a commit to cilium/hubble that referenced this issue Aug 5, 2020
Note that due to this bug[0] in the `testify` library some unit tests
started failing. See this issue[1] for more details.

[0]: stretchr/testify#930
[1]: cilium/cilium#12772

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
rolinh added a commit to cilium/hubble that referenced this issue Aug 5, 2020
Use `go-cmp` to rewrite assertion that started to fail after a cilium
dep upgrade due to a `testify` bug[0]. More details in this issue[1].

This pulls in `github.com/google/go-cmp` as an explicit dependency.

[0]: stretchr/testify#930
[1]: cilium/cilium#12772

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
rolinh added a commit to cilium/cilium that referenced this issue Aug 5, 2020
Note that due to a bug in the `testify` library[0], comparison tests for
proto generated structures are done using `go-cmp`.

[0]: stretchr/testify#930

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
rolinh added a commit to cilium/cilium that referenced this issue Aug 6, 2020
Note that due to a bug in the `testify` library[0], comparison tests for
proto generated structures are done using `go-cmp`.

[0]: stretchr/testify#930

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
tklauser pushed a commit to cilium/cilium that referenced this issue Aug 6, 2020
Note that due to a bug in the `testify` library[0], comparison tests for
proto generated structures are done using `go-cmp`.

[0]: stretchr/testify#930

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
rolinh added a commit to cilium/cilium that referenced this issue Aug 6, 2020
[ upstream commit d193b1f ]

Note that due to a bug in the `testify` library[0], comparison tests for
proto generated structures are done using `go-cmp`.

[0]: stretchr/testify#930

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
tklauser pushed a commit to cilium/cilium that referenced this issue Aug 7, 2020
[ upstream commit d193b1f ]

Note that due to a bug in the `testify` library[0], comparison tests for
proto generated structures are done using `go-cmp`.

[0]: stretchr/testify#930

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
glibsm pushed a commit to cilium/hubble that referenced this issue Aug 7, 2020
Note that due to this bug[0] in the `testify` library some unit tests
started failing. See this issue[1] for more details.

[0]: stretchr/testify#930
[1]: cilium/cilium#12772

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
glibsm pushed a commit to cilium/hubble that referenced this issue Aug 7, 2020
Use `go-cmp` to rewrite assertion that started to fail after a cilium
dep upgrade due to a `testify` bug[0]. More details in this issue[1].

This pulls in `github.com/google/go-cmp` as an explicit dependency.

[0]: stretchr/testify#930
[1]: cilium/cilium#12772

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
@LukeShu
Copy link

LukeShu commented Aug 14, 2020

Related: #758

@boyan-soubachov boyan-soubachov added this to the v2.0.0 milestone Aug 14, 2020
@mvdkleijn mvdkleijn added this to Needs triage in Bugs via automation Aug 14, 2020
@urandom
Copy link

urandom commented Sep 22, 2020

"github.com/sanity-io/litter" does not appear to suffer from the same bug as spew: https://play.golang.org/p/_1Q8xVsfLdm

Not sure how much effort, or if it is possible at all, to replace spew with this one.

@torkelrogstad
Copy link
Contributor

I think setting spew.ConfigState.MaxDepth to something reasonable is something that should be done ASAP. My tests started grinding to a halt after upgrading to the newest version of the Protobuf stack. Setting the MaxDepth to 10 worked fine for me, would that be a reasonable default? From my understanding, this would only apply to structs that are nested deeper than 10 levels, something I assume is going to be extremely rare.

@fefe982
Copy link

fefe982 commented Nov 13, 2020

I don't think this is the same problem as davecgh/go-spew#66.

davecgh/go-spew#66 is a recursion in type definition, and it does not produce output as it never gets the type's name because of the recursion in the type definition.

However, with protobuf generated message, it does generate output if I use Dump directly instead of SDump. I am not able to get a full output, but from the partial output I got, it seems that the hang here is just because the message contains too much information. I don't think there is unhandled recursion inside. Some recursion of data are correctly marked as "already shown"

I waited for a few minutes and got a partial output of more than 6.6 million lines, and found a lot of duplications inside.

@fefe982
Copy link

fefe982 commented Nov 13, 2020

If there is no easy way to fix the diff issue, can we have a new option or method, like EqualWithoutDiff, to just check the equity without output the diff?

Deep comparing the two structs are quite fast, but dumping is too slow.

@ihrwein
Copy link

ihrwein commented Dec 7, 2020

@fefe982 You could perhaps combine https://godoc.org/google.golang.org/protobuf/proto#Equal with assert.True if you don't want to see the diffs.

If you need the diffs, the protocmp package works really well as a workaround:

import (
  "github.com/google/go-cmp/cmp"
  "google.golang.org/protobuf/testing/protocmp"
)
// ...
if diff := cmp.Diff(x, y, protocmp.Transform()); diff != "" {
  t.Errorf("Unexpected difference:\n%v", diff)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Bugs
  
Needs triage
Development

No branches or pull requests

7 participants