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

proposal: sync/atomic: add JSON and Text Marshal and Unmarshal functions to scalar types #54582

Open
erikdubbelboer opened this issue Aug 22, 2022 · 7 comments

Comments

@erikdubbelboer
Copy link
Contributor

erikdubbelboer commented Aug 22, 2022

Simplified use case:

type Metrics struct {
	Requests int64 `json:"requests"` // I want to replace this with an atomic.Int64
}

func main() {
	var metrics atomic.Pointer[Metrics]

	metrics.Store(&Metrics{})

	// Standin for somehthing like a HTTP handler or something else generating metrics...
	go func() {
		for {
			time.Sleep(time.Millisecond)

			m := metrics.Load()
			atomic.AddInt64(&m.Requests, 1) // Would become m.Add(1)
		}
	}()

	for {
		time.Sleep(time.Second)

		m := metrics.Swap(&Metrics{})

		// Give other goroutines another 100 milliseconds to do
		// their atomic operations on m.
		time.Sleep(time.Millisecond * 100)

		b, _ := json.Marshal(m) // This would become safe with atomic.Int64
		fmt.Println(string(b)) // Send to some other place etc...
	}
}

Adding these methods would allow changing Metrics.Requests to an atomic.Int64 to make it simpler and safer.

The previous time this was suggested it was reject because sync/atomic can't import encoding/json. But this is not required to implement the interfaces. Only strconv would be required.

Sample implementation for atomic.Int64:

// MarshalText implements the encoding.TextMarshaler interface.
func (x *Int64) MarshalText() ([]byte, error) {
	if x == nil {
		return []byte("<nil>"), nil
	}
	return []byte(strconv.FormatInt(x.Load(), 10)), nil
}

// UnmarshalText implements the encoding.TextUnmarshaler interface.
func (x *Int64) UnmarshalText(text []byte) error {
	n, err := strconv.ParseInt(string(text), 10, 64)
	if err != nil {
		return err
	}
	x.Store(n)
	return nil
}

// MarshalJSON implements the json.Marshaler interface.
func (x *Int64) MarshalJSON() ([]byte, error) {
	if x == nil {
		return []byte("null"), nil
	}
	return x.MarshalText()
}

// UnmarshalJSON implements the json.Unmarshaler interface.
func (x *Int64) UnmarshalJSON(text []byte) error {
	// Ignore null, like in the main JSON package.
	if string(text) == "null" {
		return nil
	}
	return x.UnmarshalText(text)
}

atomic.Bool shouldn't use strconv.ParseBool as it isn't JSON compatible. Instead if can just compare against "true" and "false".

atomic.Pointer, atomic.Uintptr and atomic.Value are omitted from this proposal as they would require importing of encoding/json which is not allowed.

@gopherbot gopherbot added this to the Proposal milestone Aug 22, 2022
@dsnet
Copy link
Member

dsnet commented Aug 22, 2022

We don't need the MarshalJSON and UnmarshalJSON methods since "encoding/json" already has support for MarshalText and UnmarshalText. The only odd case is Bool, where we might need the MarshalJSON methods.

@erikdubbelboer
Copy link
Contributor Author

The problem with using MarshalText for JSON support is that it will be a string in the JSON instead of a number as expected:

type N int

func (n N) MarshalText() ([]byte, error) {
	return []byte("1234"), nil
}

type S struct {
	A N
}

func main() {
	b, _ := json.Marshal(S{})
	fmt.Println(string(b))
}
{"A":"1234"}

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@dsnet
Copy link
Member

dsnet commented Oct 12, 2022

There is some movement on taking a holistic look at all encoding/json issues. I'm a bit concerned about adding MarshalJSON methods, which are not very performant since the implementation would always have to allocate. Should we delay this until after potential changes to encoding/json stabilizes?

@dsnet
Copy link
Member

dsnet commented Oct 14, 2022

I proposed #56235, which would affect what happens here.

If that proposal is accepted, we would add something like:

func (*Bool) MarshalScalar() (bool, error)
func (*Bool) UnmarshalScalar(bool) error

func (*Int32) MarshalScalar() (int64, error)
func (*Int32) UnmarshalScalar(int64) error

func (*Int64) MarshalScalar() (int64, error)
func (*Int64) UnmarshalScalar(int64) error

func (*Uint32) MarshalScalar() (uint64, error)
func (*Uint32) UnmarshalScalar(uint64) error

func (*Uint32) MarshalScalar() (uint64, error)
func (*Uint32) UnmarshalScalar(uint64) error

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

Let's hold this proposal for #56235.

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

Placed on hold.
— rsc for the proposal review group

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

No branches or pull requests

4 participants