golang-set
golang-set copied to clipboard
deckarep
Unmarshalling threadUnsafeSet with json.Unmarshal panics
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.
@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.
@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.
@fabriziosestito - ok I am reopening and will look into a solution as my time permits.
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.
Hi, I just ran in to this issue. The workaround won't solve all cases. I want to unmarshal json with array of structures containing sets. As the number of items is unknown, I cannot initialise them in advance. I would vote for fix. It took me some time to find out that this is a known issue. I think that other future users will also appreciate it.
I would second the fix, reverting to pointer receivers. I propose the existing behavior could be split into a separate Set implementation, with UnmarshalJSON explicitly disabled (returning a NotImplementedError) with value receivers on the other methods - that way if someone needs that extra performance they can sacrifice JSON unmarshaling to get it, but the bug won't cause panics.
I third the fix. While eeking out any extra performance is nice, especially on a data structure library, I do not feel the performance tradeoff is worth keeping the bug in place, especially given @dvomartin's example of how it can become a gotcha.
