rogchap / wombat

Cross platform gRPC client
MIT License
1.4k stars 52 forks source link

Wombat (macos Catalina x86) - crashes when opening proto with imports #34

Closed JPedroGon closed 3 years ago

JPedroGon commented 3 years ago

After setting up wombat workspace towards my proto and respective imports (google api annotation and http) the application just crashes after a few seconds. It continues to happen even if I reopen it and hence impossible to use.

I also cleaned all the configurations with app cleaner to start from a fresh Wombat (latest version) and the exact same problem occurs. I can't even see the forms for the respective service. Screenshots below.

Images: https://ibb.co/DfJnJY3 https://ibb.co/j5mxqs7

rogchap commented 3 years ago

Hey @JPedroGon thanks for reporting.

Can you send your crash.log file located in you Application Support folder: ~/Library/Application Support/Wombat?

Did you upgrade from a previous (alpha) Wombat version? There was issues with the DB in early versions.

You can try deleting the Wombat folder in Application Support and restart to see if that resolves the issue?

JPedroGon commented 3 years ago

@rogchap unfortunately it is not creating a crash.log.

One of the first things I did was to clean up all the configs and folders (I used app cleaner which detected and deleted the wombat on the application support folder) but the problem still persisted. It seems to be related with imports. This started to happen when I imported google.api/annotations

rogchap commented 3 years ago

Are you able to create and send a reproducible set of protos?

I use the Google annotations with no issues.

Did you install via brew cask or GitHub releases?

JoaoSobral commented 3 years ago

Hi,

I sent you an email with the proto file. Actually this one does not have annotations and it still crashes. I used directly your release 3.6 from github, hence not via brew. I tried clean installs 3.6 and even 2.0 with same result.

rogchap commented 3 years ago

Thanks @JoaoSobral I've managed to replicate the crash at my end: stack overflow error 😱 Will debug some more and get a fix sorted out.

rogchap commented 3 years ago

So looking a bit closer at your proto files you have circular reference to the Org message:

Org -> Process -> SubProcess -> Org

Hence the stack overflow; I'm actually surprised that this is valid.

JoaoSobral commented 3 years ago

Hum you are probably right. Funny that for Gorm structs it works but I do see what you mean. In the DB is just a FK constrain but I will change my model. Basically I am injecting GORM tags on the proto as you probably saw.

JoaoSobral commented 3 years ago

Thanks for the help @rogchap ! Great tool btw 👍

rogchap commented 3 years ago

So it seems cyclic object graphs are not supported by the wireformat: https://github.com/protocolbuffers/protobuf/issues/5504

Rather than a "fix"; I'm going to detect the stack overflow to provide an error to the user (better than crashing!).

JoaoSobral commented 3 years ago

That explains it... I actually change the struct to org id and not reference the object. Wombat works again. It seems a change like this is unlikely to happen so yeah getting a error in wombat will be cool because you almost become a validation tool. Strange that protoc does not through a message when validating the proto, it should.

rogchap commented 3 years ago

35 should avoid the stack overflow crash and give you a helpful error instead:

Screen Shot 2020-12-11 at 3 57 28 pm

I'm surprised that that protoc doesn't at least warn on this 🤷‍♂️ I guess for ORM's you can use the Protobuf for the generated code and not actually use the proto wire format (ie just for code generation) so I guess it's still valid protobuf... just not used for RPCs.

JoaoSobral commented 3 years ago

That is what I am trying to avoid because then I will need to maintain two structs and create methods to copy between them. The truth is a well designed schema will not need the to have a circular reference (meaning I can reference one object or a collection but not need to revert it back to do the traditional relationships). Hence if I need to do circular reference for those specific cases I will have a struct aside. At least with this approach I can have CRUD functions that are generic in and out. Thanks @rogchap ! Btw in parallel with this I updated docker 3.0.0.0 and it also had a bug on volumes on Mac ahah so it was a buggy evening.