search

deckarep / golang-set

A simple, battle-tested and generic set type for the Go language. Trusted by Docker, 1Password, Ethereum and Hashicorp.
Other
4k stars 272 forks source link

Unmarshalling threadUnsafeSet with json.Unmarshal panics #122

Open fabriziosestito opened 11 months ago

fabriziosestito commented 11 months ago

When using json.Unmarshal to deserialize a set of type UnsafeThreadSet, a panic is raised.

Related to this comment: https://github.com/deckarep/golang-set/pull/121#issuecomment-1617551530

An example can be found here: https://go.dev/play/p/WgjZEwrHk5U

This was working prior https://github.com/deckarep/golang-set/pull/106, where the pointer receivers were refactored in favor of value receivers.

deckarep commented 11 months ago

@fabriziosestito - release 2.3.1 has been pushed. Let me know if this satisfies your fix.

@fujie-xiyou - thanks for the fix. The other issues that you mentioned in the email should all be tracked as independent Issues/PR's if you feel strongly that other things need to get addressed.

fabriziosestito commented 11 months ago

@deckarep Unfortunately the bug is still here. You can check by running https://go.dev/play/p/WgjZEwrHk5U with 2.3.1. The PR from @fujie-xiyou solves another problem about deserialization with generics. I think the confusion came from the comments in the PR mentioning this particular issue with the UnmarshalJSON.

The only solution I see here is to go back to pointer receivers, otherwise, UnmarshalJSON will not work as it needs to mutate the data.

I cannot re-open this issue.

deckarep commented 11 months ago

@fabriziosestito - ok I am reopening and will look into a solution as my time permits.

deckarep commented 6 months ago

I finally got around to looking into this: It's a somewhat nasty bug and the best explanation I have so far: pretty much in all cases when the UnmarshalJSON interface is implemented it must be implemented on a pointer-based receiver. The json decoder needs to be able to reassign what it points in some cases so this is a hard requirement. Currently to satisfy it being implemented on the Set interface it's established as a value receiver for the threadUnsafe flavor, after a somewhat recent refactor as @fabriziosestito noticed.

As it stands, although the test case involved, as posted in the playground link; takes the address of the set object which is a generic interface, the decoder isn't able to resolve that the UnmarshalJSON exists on a pointer-based receiver because it is indeed not present.

Changing the test to explicitly use the UnmarshalJSON method by calling: data.UnmarshalJSON fixes the issue (as a workaround) but having it called via: json.Unmarshal indirectly is where the failure happens.

I don't know how often folks are using the Set with encoding/decoding JSON but I'm curious how others might feel.

As it stands there is a very simple workaround.

Otherwise, I think the only option is to go back to moving the entirety of all threadUnsafe set methods to be pointer based as it was before. This will fix the issue at the cost of having the slightest more overhead due to extra indirection.