api: updated response/future methods

[ermyar]

Added Response's interface method Buffer.
Connection now contains sync.Pool that was used to reduce allocations.

Fixes #493

I didn't forget about (remove if it is not applicable):

• Tests (see documentation for a testing package)
• Changelog (see documentation for changelog format)

[ermyar]

Detected flaky TestSQLBindings and TestStressSQL

[oleg-jukovec]

Thank you for the patch!

See issues of the current implementation, fix them and we will try again.

[oleg-jukovec]

You've actually return a pointer into a local value (address of ptr) instead of an original slice that was allocated at a pool.

https://go.dev/play/p/MsjOHvigpnI

[oleg-jukovec]

The release method must free all data allocated by the response. Otherwise, the semantic of the call is very confusing.

[ermyar]

ofc, we could design better

but if we want to keep to each connection their own sync.Pool, so we should provide some relations between response and connection (to do this, i used future and future.conn). Or could we single pool for all connections?

[ermyar]

oh, i understand my fault, sorry..

[oleg-jukovec]

it is shorter, but still not work as expected.

Suggested change

	resp.header.RequestId, resp.header.Error = 0, 0
	resp.data = nil
	ptr := resp.buf.b
	resp.buf.b = nil
	resp.buf.p = 0
	resp.decoded = false
	resp.decodedTyped = false
	resp.err = nil
	ptr := resp.buf.b
	*resp = SelectResponse{}

[ermyar]

but we are allocating here a new SelectResponse object, aren't we?

(and this is a baseResponse's method, why do we use SelectResponse?)

[oleg-jukovec]

but we are allocating here a new SelectResponse object, aren't we?

No, nothing will be allocated here.

(and this is a baseResponse's method, why do we use SelectResponse?)

My fault, should be baseResponse{}.

[oleg-jukovec]

This looks like overengineering. We don't have any allocations of the baseResponse objects. And therefore all this stuff about baseResponse is not needed at all.

[oleg-jukovec]

It should look like this (all resources related to a response should be release inside response.Release()):

Suggested change

	// Release is freeing the Future resources.
	// After this, using this Future becomes invalid.
	func (fut *Future) Release() {
	if fut.pool != nil && fut.resp != nil {
	ptr := fut.resp.Release()
	fut.pool.Put(ptr)
	}
	}
	// Release is freeing the Future resources.
	// After this, using this Future becomes invalid.
	func (fut *Future) Release() {
	if fut.resp != nil {
	fut.resp.Release()
	}
	}

[oleg-jukovec]

Suggested change

	func (fut *Future) Get() ([]interface{}, error) {
	defer fut.Release()
	func (fut *Future) Get() ([]interface{}, error) {

The code:

fut := conn.Do(req)
data, err := fut.Get()
data, err := fut.Get()

Should work fine by design. A user should call future.Release() after usage of the future directly:

fut := conn.Do(req)
data, err := fut.Get()
data, err := fut.Get()
fut.Release()

[oleg-jukovec]

Suggested change

	defer fut.Release()
	fut.wait()
	fut.wait()

[oleg-jukovec]

It will not work since the same object will be used inside multiple futures:

go-tarantool/connection.go

Line 895 in bf0bbdc

if err := fut.SetResponse(header, &buf); err != nil {

The simplest solution - a pool of smallBuf objects.