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

Optional support of msgpack.v5 #174

Merged
merged 7 commits into from
Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions .github/workflows/testing.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,15 @@ jobs:
- name: Run regression tests
run: make test

- name: Run tests with call_17
- name: Run regression tests with call_17
run: make test TAGS="go_tarantool_call_17"

- name: Run regression tests with msgpack.v5
run: make test TAGS="go_tarantool_msgpack_v5"

- name: Run regression tests with msgpack.v5 and call_17
run: make test TAGS="go_tarantool_msgpack_v5,go_tarantool_call_17"

- name: Run fuzzing tests
if: ${{ matrix.fuzzing }}
run: make fuzzing TAGS="go_tarantool_decimal_fuzzing"
Expand Down Expand Up @@ -146,20 +152,34 @@ jobs:
source tarantool-enterprise/env.sh
make deps

- name: Run tests
- name: Run regression tests
run: |
source tarantool-enterprise/env.sh
make test
env:
TEST_TNT_SSL: ${{matrix.ssl}}

- name: Run tests with call_17
- name: Run regression tests with call_17
run: |
source tarantool-enterprise/env.sh
make test TAGS="go_tarantool_call_17"
env:
TEST_TNT_SSL: ${{matrix.ssl}}

- name: Run regression tests with msgpack.v5
run: |
source tarantool-enterprise/env.sh
make test TAGS="go_tarantool_msgpack_v5"
env:
TEST_TNT_SSL: ${{matrix.ssl}}

- name: Run regression tests with msgpack.v5 and call_17
run: |
source tarantool-enterprise/env.sh
make test TAGS="go_tarantool_msgpack_v5,go_tarantool_call_17"
env:
TEST_TNT_SSL: ${{matrix.ssl}}

- name: Run fuzzing tests
if: ${{ matrix.fuzzing }}
run: make fuzzing TAGS="go_tarantool_decimal_fuzzing"
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release.

### Added

- Optional msgpack.v5 usage (#124)

### Changed

### Fixed
Expand Down
5 changes: 5 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ make test
The tests set up all required `tarantool` processes before run and clean up
afterwards.

If you want to run the tests with specific build tags:
```bash
make test TAGS=go_tarantool_ssl_disable,go_tarantool_msgpack_v5
```

If you have Tarantool Enterprise Edition 2.10 or newer, you can run additional
SSL tests. To do this, you need to set an environment variable 'TEST_TNT_SSL':

Expand Down
36 changes: 35 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ faster than other packages according to public benchmarks.
* [Documentation](#documentation)
* [API reference](#api-reference)
* [Walking\-through example](#walking-through-example)
* [msgpack.v5 migration](#msgpackv5-migration)
* [Contributing](#contributing)
* [Alternative connectors](#alternative-connectors)

Expand Down Expand Up @@ -68,7 +69,15 @@ This allows us to introduce new features without losing backward compatibility.
go_tarantool_call_17
```
**Note:** In future releases, `Call17` may be used as default `Call` behavior.
3. To run fuzz tests with decimals, you can use the build tag:
3. To replace usage of `msgpack.v2` with `msgpack.v5`, you can use the build
tag:
```
go_tarantool_msgpack_v5
```
**Note:** In future releases, `msgpack.v5` may be used by default. We recommend
to read [msgpack.v5 migration notes](#msgpackv5-migration) and try to
use msgpack.v5 before the changes.
4. To run fuzz tests with decimals, you can use the build tag:
```
go_tarantool_decimal_fuzzing
```
Expand Down Expand Up @@ -144,6 +153,31 @@ There are two parameters:
* a space number (it could just as easily have been a space name), and
* a tuple.

### msgpack.v5 migration

Most function names and argument types in `msgpack.v5` and `msgpack.v2`
have not changed (in our code, we noticed changes in `EncodeInt`, `EncodeUint`
and `RegisterExt`). But there are a lot of changes in a logic of encoding and
decoding. On the plus side the migration seems easy, but on the minus side you
need to be very careful.

First of all, `EncodeInt8`, `EncodeInt16`, `EncodeInt32`, `EncodeInt64`
and `EncodeUint*` analogues at `msgpack.v5` encode numbers as is without loss of
type. In `msgpack.v2` the type of a number is reduced to a value.

Secondly, a base decoding function does not convert numbers to `int64` or
`uint64`. It converts numbers to an exact type defined by MessagePack. The
change makes manual type conversions much more difficult and can lead to
runtime errors with an old code. We do not recommend to use type conversions
and give preference to `*Typed` functions (besides, it's faster).

There are also changes in the logic that can lead to errors in the old code,
[as example](https://github.com/vmihailenco/msgpack/issues/327). Although in
`msgpack.v5` some functions for the logic tuning were added (see
[UseLooseInterfaceDecoding](https://pkg.go.dev/github.com/vmihailenco/msgpack/v5#Decoder.UseLooseInterfaceDecoding), [UseCompactInts](https://pkg.go.dev/github.com/vmihailenco/msgpack/v5#Encoder.UseCompactInts) etc), it is still impossible
to achieve full compliance of behavior between `msgpack.v5` and `msgpack.v2`. So
we don't go this way. We use standart settings if it possible.

## Contributing

See [the contributing guide](CONTRIBUTING.md) for detailed instructions on how
Expand Down
10 changes: 5 additions & 5 deletions call_16_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ func TestConnection_Call(t *testing.T) {
defer conn.Close()

// Call16
resp, err = conn.Call("simple_incr", []interface{}{1})
resp, err = conn.Call("simple_concat", []interface{}{"1"})
DifferentialOrange marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
t.Errorf("Failed to use Call")
}
if resp.Data[0].([]interface{})[0].(uint64) != 2 {
if val, ok := resp.Data[0].([]interface{})[0].(string); !ok || val != "11" {
t.Errorf("result is not {{1}} : %v", resp.Data)
}
}
Expand All @@ -34,18 +34,18 @@ func TestCallRequest(t *testing.T) {
conn := test_helpers.ConnectWithValidation(t, server, opts)
defer conn.Close()

req := NewCallRequest("simple_incr").Args([]interface{}{1})
req := NewCallRequest("simple_concat").Args([]interface{}{"1"})
resp, err = conn.Do(req).Get()
if err != nil {
t.Errorf("Failed to use Call")
}
if resp.Data[0].([]interface{})[0].(uint64) != 2 {
if val, ok := resp.Data[0].([]interface{})[0].(string); !ok || val != "11" {
t.Errorf("result is not {{1}} : %v", resp.Data)
}
}

func TestCallRequestCode(t *testing.T) {
req := NewCallRequest("simple_incrt")
req := NewCallRequest("simple_concat")
code := req.Code()
expected := Call16RequestCode
if code != int32(expected) {
Expand Down
10 changes: 5 additions & 5 deletions call_17_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ func TestConnection_Call(t *testing.T) {
defer conn.Close()

// Call17
resp, err = conn.Call17("simple_incr", []interface{}{1})
resp, err = conn.Call17("simple_concat", []interface{}{"1"})
if err != nil {
t.Errorf("Failed to use Call")
}
if resp.Data[0].(uint64) != 2 {
if val, ok := resp.Data[0].(string); !ok || val != "11" {
t.Errorf("result is not {{1}} : %v", resp.Data)
}
}
Expand All @@ -34,18 +34,18 @@ func TestCallRequest(t *testing.T) {
conn := test_helpers.ConnectWithValidation(t, server, opts)
defer conn.Close()

req := NewCallRequest("simple_incr").Args([]interface{}{1})
req := NewCallRequest("simple_concat").Args([]interface{}{"1"})
resp, err = conn.Do(req).Get()
if err != nil {
t.Errorf("Failed to use Call")
}
if resp.Data[0].(uint64) != 2 {
if val, ok := resp.Data[0].(string); !ok || val != "11" {
t.Errorf("result is not {{1}} : %v", resp.Data)
}
}

func TestCallRequestCode(t *testing.T) {
req := NewCallRequest("simple_incrt")
req := NewCallRequest("simple_concat")
code := req.Code()
expected := Call17RequestCode
if code != int32(expected) {
Expand Down
44 changes: 20 additions & 24 deletions client_tools.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
package tarantool

import (
"gopkg.in/vmihailenco/msgpack.v2"
)

// IntKey is utility type for passing integer key to Select*, Update* and Delete*.
// It serializes to array with single integer element.
type IntKey struct {
I int
}

DifferentialOrange marked this conversation as resolved.
Show resolved Hide resolved
func (k IntKey) EncodeMsgpack(enc *msgpack.Encoder) error {
enc.EncodeSliceLen(1)
enc.EncodeInt(k.I)
func (k IntKey) EncodeMsgpack(enc *encoder) error {
enc.EncodeArrayLen(1)
encodeInt(enc, int64(k.I))
return nil
}

Expand All @@ -22,9 +18,9 @@ type UintKey struct {
I uint
}

func (k UintKey) EncodeMsgpack(enc *msgpack.Encoder) error {
enc.EncodeSliceLen(1)
enc.EncodeUint(k.I)
func (k UintKey) EncodeMsgpack(enc *encoder) error {
enc.EncodeArrayLen(1)
encodeUint(enc, uint64(k.I))
return nil
}

Expand All @@ -34,8 +30,8 @@ type StringKey struct {
S string
}

func (k StringKey) EncodeMsgpack(enc *msgpack.Encoder) error {
enc.EncodeSliceLen(1)
func (k StringKey) EncodeMsgpack(enc *encoder) error {
enc.EncodeArrayLen(1)
enc.EncodeString(k.S)
return nil
}
Expand All @@ -46,10 +42,10 @@ type IntIntKey struct {
I1, I2 int
}

func (k IntIntKey) EncodeMsgpack(enc *msgpack.Encoder) error {
enc.EncodeSliceLen(2)
enc.EncodeInt(k.I1)
enc.EncodeInt(k.I2)
func (k IntIntKey) EncodeMsgpack(enc *encoder) error {
enc.EncodeArrayLen(2)
encodeInt(enc, int64(k.I1))
encodeInt(enc, int64(k.I2))
return nil
}

Expand All @@ -60,10 +56,10 @@ type Op struct {
Arg interface{}
}

func (o Op) EncodeMsgpack(enc *msgpack.Encoder) error {
enc.EncodeSliceLen(3)
func (o Op) EncodeMsgpack(enc *encoder) error {
enc.EncodeArrayLen(3)
enc.EncodeString(o.Op)
enc.EncodeInt(o.Field)
encodeInt(enc, int64(o.Field))
return enc.Encode(o.Arg)
}

Expand Down Expand Up @@ -148,12 +144,12 @@ type OpSplice struct {
Replace string
}

func (o OpSplice) EncodeMsgpack(enc *msgpack.Encoder) error {
enc.EncodeSliceLen(5)
func (o OpSplice) EncodeMsgpack(enc *encoder) error {
enc.EncodeArrayLen(5)
enc.EncodeString(o.Op)
enc.EncodeInt(o.Field)
enc.EncodeInt(o.Pos)
enc.EncodeInt(o.Len)
encodeInt(enc, int64(o.Field))
encodeInt(enc, int64(o.Pos))
encodeInt(enc, int64(o.Len))
enc.EncodeString(o.Replace)
return nil
}
8 changes: 4 additions & 4 deletions config.lua
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ box.once("init", function()

--box.schema.user.grant('guest', 'read,write,execute', 'universe')
box.schema.func.create('box.info')
box.schema.func.create('simple_incr')
box.schema.func.create('simple_concat')

-- auth testing: access control
box.schema.user.create('test', {password = 'test'})
Expand All @@ -106,10 +106,10 @@ local function func_name()
end
rawset(_G, 'func_name', func_name)

local function simple_incr(a)
return a + 1
local function simple_concat(a)
return a .. a
end
rawset(_G, 'simple_incr', simple_incr)
rawset(_G, 'simple_concat', simple_concat)

local function push_func(cnt)
for i = 1, cnt do
Expand Down
14 changes: 6 additions & 8 deletions connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ import (
"sync"
"sync/atomic"
"time"

"gopkg.in/vmihailenco/msgpack.v2"
)

const requestsMap = 128
Expand Down Expand Up @@ -142,7 +140,7 @@ type Connection struct {
rlimit chan struct{}
opts Opts
state uint32
dec *msgpack.Decoder
dec *decoder
lenbuf [PacketLengthBytes]byte

lastStreamId uint64
Expand Down Expand Up @@ -199,7 +197,7 @@ type connShard struct {
requestsWithCtx [requestsMap]futureList
bufmut sync.Mutex
buf smallWBuf
enc *msgpack.Encoder
enc *encoder
}

// Greeting is a message sent by Tarantool on connect.
Expand Down Expand Up @@ -320,7 +318,7 @@ func Connect(addr string, opts Opts) (conn *Connection, err error) {
Greeting: &Greeting{},
control: make(chan struct{}),
opts: opts,
dec: msgpack.NewDecoder(&smallBuf{}),
dec: newDecoder(&smallBuf{}),
}
maxprocs := uint32(runtime.GOMAXPROCS(-1))
if conn.opts.Concurrency == 0 || conn.opts.Concurrency > maxprocs*128 {
Expand Down Expand Up @@ -531,7 +529,7 @@ func (conn *Connection) dial() (err error) {
return
}

func pack(h *smallWBuf, enc *msgpack.Encoder, reqid uint32,
func pack(h *smallWBuf, enc *encoder, reqid uint32,
req Request, streamId uint64, res SchemaResolver) (err error) {
hl := h.Len()

Expand Down Expand Up @@ -569,7 +567,7 @@ func pack(h *smallWBuf, enc *msgpack.Encoder, reqid uint32,
func (conn *Connection) writeAuthRequest(w *bufio.Writer, scramble []byte) (err error) {
var packet smallWBuf
req := newAuthRequest(conn.opts.User, string(scramble))
err = pack(&packet, msgpack.NewEncoder(&packet), 0, req, ignoreStreamId, conn.Schema)
err = pack(&packet, newEncoder(&packet), 0, req, ignoreStreamId, conn.Schema)

if err != nil {
return errors.New("auth: pack error " + err.Error())
Expand Down Expand Up @@ -916,7 +914,7 @@ func (conn *Connection) putFuture(fut *Future, req Request, streamId uint64) {
firstWritten := shard.buf.Len() == 0
if shard.buf.Cap() == 0 {
shard.buf.b = make([]byte, 0, 128)
shard.enc = msgpack.NewEncoder(&shard.buf)
shard.enc = newEncoder(&shard.buf)
}
blen := shard.buf.Len()
reqid := fut.requestId
Expand Down
Loading