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

time.Time is not follow the msgpack spec. #300

Closed
piyongcai opened this issue Oct 16, 2021 · 7 comments · Fixed by #378
Closed

time.Time is not follow the msgpack spec. #300

piyongcai opened this issue Oct 16, 2021 · 7 comments · Fixed by #378

Comments

@piyongcai
Copy link

in msgpak https://github.com/msgpack/msgpack/blob/master/spec.md

Timestamp extension type
Timestamp extension type is assigned to extension type -1. It defines 3 formats: 32-bit format, 64-bit format, and 96-bit format.

timestamp 32 stores the number of seconds that have elapsed since 1970-01-01 00:00:00 UTC
in an 32-bit unsigned integer:
+--------+--------+--------+--------+--------+--------+
| 0xd6 | -1 | seconds in 32-bit unsigned int |
+--------+--------+--------+--------+--------+--------+

timestamp 64 stores the number of seconds and nanoseconds that have elapsed since 1970-01-01 00:00:00 UTC
in 32-bit unsigned integers:
+--------+--------+--------+--------+--------+------|-+--------+--------+--------+--------+
| 0xd7 | -1 | nanosec. in 30-bit unsigned int | seconds in 34-bit unsigned int |
+--------+--------+--------+--------+--------+------^-+--------+--------+--------+--------+

timestamp 96 stores the number of seconds and nanoseconds that have elapsed since 1970-01-01 00:00:00 UTC
in 64-bit signed integer and 32-bit unsigned integer:
+--------+--------+--------+--------+--------+--------+--------+
| 0xc7 | 12 | -1 |nanoseconds in 32-bit unsigned int |
+--------+--------+--------+--------+--------+--------+--------+
+--------+--------+--------+--------+--------+--------+--------+--------+
seconds in 64-bit signed int |
+--------+--------+--------+--------+--------+--------+--------+--------+

but in the tinylib/msgp, time.Time Marash func is not follow the spec.

func putUnix(b []byte, sec int64, nsec int32) {
	b[0] = byte(sec >> 56)
	b[1] = byte(sec >> 48)
	b[2] = byte(sec >> 40)
	b[3] = byte(sec >> 32)
	b[4] = byte(sec >> 24)
	b[5] = byte(sec >> 16)
	b[6] = byte(sec >> 8)
	b[7] = byte(sec)
	b[8] = byte(nsec >> 24)
	b[9] = byte(nsec >> 16)
	b[10] = byte(nsec >> 8)
	b[11] = byte(nsec)
}
@klauspost
Copy link
Collaborator

The extension code used by msgp is 5, not -1, so it doesn't have to follow the predefined time type.

According to the docs you can annotate with msg:"name,extension" and register your new type.

This package predates the time extension proposal so that is probably why it is like this.

@sonirico
Copy link

sonirico commented Jul 18, 2022

I've bumped into this and I would like to understand. The current implementation does not provide interoperability between other libraries such as https://www.npmjs.com/package/msgpack5, for instance. In this case, msgpack5 for node yields Error: unable to find ext type 5 when trying to deserialize time.Time types serialized by msgp. I am very surprised that this is happening... and not so people are complaining. Other libs like github.com/vmihailenco/msgpack/v5 do not incur in this error.

I understand that the implementation chosen here might be different in order to strongly adhere to specs, but how is the global situation and the motivation to do this?

@klauspost
Copy link
Collaborator

klauspost commented Jul 19, 2022

//msgp:ignore Time

// Time is a time.Time that serializes to/from Msgpack Extension -1.
type Time struct {
	time.Time
}

func (t Time) ExtensionType() int8 {
	return -1
}

func (t Time) Len() int {
	// Time round towards zero time.
	secPrec := t.Round(time.Second)
	remain := t.Sub(secPrec)
	asSecs := secPrec.Unix()
	if remain == 0 && asSecs > 0 && asSecs <= math.MaxUint32 {
		return 4
	}
	if asSecs < 0 || asSecs >= (1<<34) {
		return 12
	}
	return 8
}

func (t Time) MarshalBinaryTo(bytes []byte) error {
	// Time rounded towards zero.
	secPrec := t.Truncate(time.Second)
	remain := t.Sub(secPrec).Nanoseconds()
	asSecs := secPrec.Unix()
	if remain == 0 && asSecs > 0 && asSecs <= math.MaxUint32 {
		if len(bytes) != 4 {
			return fmt.Errorf("expected length 4, got %d", len(bytes))
		}
		binary.BigEndian.PutUint32(bytes, uint32(asSecs))
		return nil
	}
	if asSecs < 0 || asSecs >= (1<<34) {
		if len(bytes) != 12 {
			return fmt.Errorf("expected length 12, got %d", len(bytes))
		}
		binary.BigEndian.PutUint32(bytes[:4], uint32(remain))
		binary.BigEndian.PutUint64(bytes[4:], uint64(asSecs))
		return nil
	}
	if len(bytes) != 8 {
		return fmt.Errorf("expected length 8, got %d", len(bytes))
	}
	binary.BigEndian.PutUint64(bytes, uint64(asSecs)|(uint64(remain)<<34))
	return nil
}

func (t *Time) UnmarshalBinary(bytes []byte) error {
	switch len(bytes) {
	case 4:
		secs := binary.BigEndian.Uint32(bytes)
		t.Time = time.Unix(int64(secs), 0)
		return nil
	case 8:
		data64 := binary.BigEndian.Uint64(bytes)
		nsecs := int64(data64 >> 34)
		if nsecs > 999999999 {
			// In timestamp 64 and timestamp 96 formats, nanoseconds must not be larger than 999999999.
			return fmt.Errorf("nsecs overflow")
		}
		secs := int64(data64 & 0x3ffffffff)
		t.Time = time.Unix(secs, nsecs)
		return nil
	case 12:
		nsecs := int64(binary.BigEndian.Uint32(bytes[:4]))
		if nsecs > 999999999 {
			// In timestamp 64 and timestamp 96 formats, nanoseconds must not be larger than 999999999.
			return fmt.Errorf("nsecs overflow")
		}
		secs := int64(binary.BigEndian.Uint64(bytes[4:]))
		t.Time = time.Unix(secs, nsecs)
		return nil
	}
	return fmt.Errorf("unknown time format length: %v", len(bytes))
}

The spec doesn't specify explicitly that numbers are big endian, but since the other msgpack are, I assume they are.

Annotate with ",extension", eg:

	Modtime Time   `msg:"Modtime,extension"`.
Roundtrip Test

func TestTime_MarshalBinaryTo(t *testing.T) {
	rng := rand.New(rand.NewSource(0))
	for i := 0; i < 10000; i++ {
		in := Time{
			Time: time.Unix(rng.Int63()-1<<62, rng.Int63n(999999999+1)),
		}
		dst := make([]byte, in.Len())
		err := in.MarshalBinaryTo(dst)
		if err != nil {
			t.Fatal(err)
		}
		var got Time
		err = got.UnmarshalBinary(dst)
		if err != nil {
			t.Fatal(err)
		}
		if !in.Equal(got.Time) {
			t.Errorf("%v != %v", in.Time, got.Time)
		}
	}
	for i := 0; i < 10000; i++ {
		in := Time{
			Time: time.Unix(rng.Int63n(1<<32), 0),
		}
		dst := make([]byte, in.Len())
		if len(dst) != 4 {
			t.Errorf("unexpected length: %v", len(dst))
		}
		err := in.MarshalBinaryTo(dst)
		if err != nil {
			t.Fatal(err)
		}
		var got Time
		err = got.UnmarshalBinary(dst)
		if err != nil {
			t.Fatal(err)
		}
		if !in.Equal(got.Time) {
			t.Errorf("%v != %v", in.Time, got.Time)
		}
	}
	for i := 0; i < 10000; i++ {
		in := Time{
			Time: time.Unix(rng.Int63n(1<<34), rng.Int63n(999999999+1)),
		}
		dst := make([]byte, in.Len())
		if len(dst) > 8 {
			t.Errorf("unexpected length: %v", len(dst))
		}
		err := in.MarshalBinaryTo(dst)
		if err != nil {
			t.Fatal(err)
		}
		var got Time
		err = got.UnmarshalBinary(dst)
		if err != nil {
			t.Fatal(err)
		}
		if !in.Equal(got.Time) {
			t.Errorf("%v != %v", in.Time, got.Time)
		}
	}
}

@sonirico
Copy link

@klauspost you certainly were quick in answering and providing this solution! Is there any repo with extensions like this?

Thanks so much!

@sonirico
Copy link

sonirico commented Jul 26, 2022

@klauspost I made as you pointed, but now I get this error when running tests (and thus running msgp autogenerated tests): EOF. Debugging reached me to this function of the fwd package.

// ReadMapHeader reads the next object
// as a map header and returns the size
// of the map and the number of bytes written.
// It will return a TypeError{} if the next
// object is not a map.
func (m *Reader) ReadMapHeader() (sz uint32, err error) {
}

My time struct is defined as follows:

//msgp:ignore Time

// Time is a time.Time that serializes to/from Msgpack Extension -1.
type Time struct {
	time.Time
}

func (t Time) Equal(other Time) bool {
	return t.Time.Equal(other.Time)
}

func (t Time) FromTime(native time.Time) Time {
	t.Time = native
	return t
}

func (t Time) Truncate(d time.Duration) Time {
	return Time{Time: t.Time.Truncate(d)}
}

func (t Time) Add(d time.Duration) Time {
	return Time{Time: t.Time.Add(d)}
}

func (t Time) ExtensionType() int8 {
	return -1
}

func (t Time) Len() int {
	// Time round towards zero time.
	secPrec := t.Round(time.Second)
	remain := t.Sub(secPrec)
	asSecs := secPrec.Unix()
	if remain == 0 && asSecs > 0 && asSecs <= math.MaxUint32 {
		return 4
	}
	if asSecs < 0 || asSecs >= (1<<34) {
		return 12
	}
	return 8
}

func (t Time) MarshalBinaryTo(bytes []byte) error {
	// Time rounded towards zero.
	secPrec := t.Time.Truncate(time.Second)
	remain := t.Sub(secPrec).Nanoseconds()
	asSecs := secPrec.Unix()
	if remain == 0 && asSecs > 0 && asSecs <= math.MaxUint32 {
		if len(bytes) != 4 {
			return fmt.Errorf("expected length 4, got %d", len(bytes))
		}
		binary.BigEndian.PutUint32(bytes, uint32(asSecs))
		return nil
	}
	if asSecs < 0 || asSecs >= (1<<34) {
		if len(bytes) != 12 {
			return fmt.Errorf("expected length 12, got %d", len(bytes))
		}
		binary.BigEndian.PutUint32(bytes[:4], uint32(remain))
		binary.BigEndian.PutUint64(bytes[4:], uint64(asSecs))
		return nil
	}
	if len(bytes) != 8 {
		return fmt.Errorf("expected length 8, got %d", len(bytes))
	}
	binary.BigEndian.PutUint64(bytes, uint64(asSecs)|(uint64(remain)<<34))
	return nil
}

func (t *Time) UnmarshalBinary(bytes []byte) error {
	switch len(bytes) {
	case 4:
		secs := binary.BigEndian.Uint32(bytes)
		t.Time = time.Unix(int64(secs), 0)
		return nil
	case 8:
		data64 := binary.BigEndian.Uint64(bytes)
		nsecs := int64(data64 >> 34)
		if nsecs > 999999999 {
			// In timestamp 64 and timestamp 96 formats, nanoseconds must not be larger than 999999999.
			return fmt.Errorf("nsecs overflow")
		}
		secs := int64(data64 & 0x3ffffffff)
		t.Time = time.Unix(secs, nsecs)
		return nil
	case 12:
		nsecs := int64(binary.BigEndian.Uint32(bytes[:4]))
		if nsecs > 999999999 {
			// In timestamp 64 and timestamp 96 formats, nanoseconds must not be larger than 999999999.
			return fmt.Errorf("nsecs overflow")
		}
		secs := int64(binary.BigEndian.Uint64(bytes[4:]))
		t.Time = time.Unix(secs, nsecs)
		return nil
	}
	return fmt.Errorf("unknown time format length: %v", len(bytes))
}

func init() {
	// Registering an extension is as simple as matching the
	// appropriate type number with a function that initializes
	// a freshly-allocated object of that type
	msgp.RegisterExtension(-1, func() msgp.Extension { return new(Time) })
}

and the model is

	Model struct {
		Time   Time    `json:"time"         msg:"time,extension"`
		//... some other fields
	}

@klauspost
Copy link
Collaborator

Seems like MarshalBinaryTo cannot expect to be given the exact output size. Fixed:

func (t Time) MarshalBinaryTo(bytes []byte) error {
	// Time rounded towards zero.
	secPrec := t.Time.Truncate(time.Second)
	remain := t.Sub(secPrec).Nanoseconds()
	asSecs := secPrec.Unix()
	if remain == 0 && asSecs > 0 && asSecs <= math.MaxUint32 {
		if len(bytes) < 4 {
			return fmt.Errorf("need length 4, got %d", len(bytes))
		}
		binary.BigEndian.PutUint32(bytes, uint32(asSecs))
		return nil
	}
	if asSecs < 0 || asSecs >= (1<<34) {
		if len(bytes) < 12 {
			return fmt.Errorf("need length 12, got %d", len(bytes))
		}
		binary.BigEndian.PutUint32(bytes[:4], uint32(remain))
		binary.BigEndian.PutUint64(bytes[4:], uint64(asSecs))
		return nil
	}
	if len(bytes) < 8 {
		return fmt.Errorf("need length 8, got %d", len(bytes))
	}
	binary.BigEndian.PutUint64(bytes, uint64(asSecs)|(uint64(remain)<<34))
	return nil
}

klauspost added a commit to klauspost/msgp that referenced this issue Oct 29, 2024
This adds `msgp:newtime` file directive that will encode all time fields using the -1 extension as defined in the [(revised) messagepack spec](https://github.com/msgpack/msgpack/blob/master/spec.md#timestamp-extension-type)

ReadTime/ReadTimeBytes will now support both types natively, and will accept either as input.

Extensions should remain unaffected.

Fixes tinylib#300
@klauspost
Copy link
Collaborator

File directive is added in #378

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

Successfully merging a pull request may close this issue.

3 participants