Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup
@ 2020-02-15 18:08 Vladislav Shpilevoy
  2020-02-15 18:08 ` [Tarantool-patches] [PATCH 1/2] tuple: hide internal functions from box.tuple.* Vladislav Shpilevoy
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-15 18:08 UTC (permalink / raw)
  To: tarantool-patches, babinoleg, alexander.turenko, imun

The patchset removes or documents some internal functions in
box.tuple.* namespace: box.tuple.bless()/encode()/is().

Bless() and encode() were moved to a more secret place, where
suicidal users can't find it.

Is() is documented, because it is actually a useful thing.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4662-fiber-storage-leak
Issue: https://github.com/tarantool/tarantool/issues/4662

Vladislav Shpilevoy (2):
  tuple: hide internal functions from box.tuple.*
  tuple: make box.tuple.is() public

 src/box/lua/schema.lua  |  4 ++--
 src/box/lua/tuple.lua   | 15 +++++++++++++--
 test/box/tuple.result   | 40 ++++++++++++++++++++++++++++++++++++++++
 test/box/tuple.test.lua | 14 ++++++++++++++
 4 files changed, 69 insertions(+), 4 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Tarantool-patches] [PATCH 1/2] tuple: hide internal functions from box.tuple.*
  2020-02-15 18:08 [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Vladislav Shpilevoy
@ 2020-02-15 18:08 ` Vladislav Shpilevoy
  2020-03-16 13:15   ` Nikita Pettik
  2020-02-15 18:08 ` [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public Vladislav Shpilevoy
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-15 18:08 UTC (permalink / raw)
  To: tarantool-patches, babinoleg, alexander.turenko, imun

box.tuple.bless, .encode, and .is are internal. Their
behaviour is not documented, and they may omit some checks for the
sake of speed, and can crash if used without thinking.

Nonetheless, despite they are not documented, curious users could
notice them in box.tuple.* output via autocompletion, for example.
And they could try to use them. This is not ok.

box.tuple.bless() being called by a user leads either to a crash,
or to a leak (because it is basically tuple reference counter
increment).

box.tuple.encode() is kind of a wrapper around msgpack, and users
should not touch it. It may change, may be removed. And is just
makes no sense except some rare cases in schema.lua.

bless() and encode() were used in schema.lua only, so the patch
simply moves them to box.internal.tuple.

box.tuple.is() is kept as is, because
- this is used in the tests a lot;
- it is totally safe;
- that function actually makes sense, and some users could have
  already started using it.

There is no a test, since nothing to test - bless() is not
available for users anymore (assuming no one is brave enough to
rely on box.internal).

Closes #4684
---
 src/box/lua/schema.lua |  4 ++--
 src/box/lua/tuple.lua  | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 50c96a335..0d28edbe6 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -17,8 +17,8 @@ end
 local builtin = ffi.C
 
 -- performance fixup for hot functions
-local tuple_encode = box.tuple.encode
-local tuple_bless = box.tuple.bless
+local tuple_encode = box.internal.tuple.encode
+local tuple_bless = box.internal.tuple.bless
 local is_tuple = box.tuple.is
 assert(tuple_encode ~= nil and tuple_bless ~= nil and is_tuple ~= nil)
 
diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
index a25a28987..eb3946a0f 100644
--- a/src/box/lua/tuple.lua
+++ b/src/box/lua/tuple.lua
@@ -342,7 +342,17 @@ ffi.metatype(tuple_iterator_t, {
     __tostring = function(it) return "<tuple iterator>" end;
 })
 
+-- Free methods, which are not needed anymore.
+internal.tuple.slice = nil
+internal.tuple.transform = nil
+internal.tuple.tuple_to_map = nil
+internal.tuple.tostring = nil
+
 -- internal api for box.select and iterators
-box.tuple.bless = tuple_bless
-box.tuple.encode = tuple_encode
+internal.tuple.bless = tuple_bless
+internal.tuple.encode = tuple_encode
+
+-- The function is internal in a sense that it is not documented.
+-- But it is safe and widely used in the tests. Keep it here at
+-- least for test code.
 box.tuple.is = is_tuple
-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public
  2020-02-15 18:08 [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Vladislav Shpilevoy
  2020-02-15 18:08 ` [Tarantool-patches] [PATCH 1/2] tuple: hide internal functions from box.tuple.* Vladislav Shpilevoy
@ 2020-02-15 18:08 ` Vladislav Shpilevoy
  2020-02-16 15:07   ` Vladislav Shpilevoy
  2020-03-16 13:19   ` Nikita Pettik
  2020-02-15 19:02 ` [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Oleg Babin
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-15 18:08 UTC (permalink / raw)
  To: tarantool-patches, babinoleg, alexander.turenko, imun

In #4684 it was found that box.tuple.* contained some private
functions: bless(), encode(), and is().

Bless() and encode() didn't make any sense for a user, so they
were hidden into box.internal.tuple.*.

But box.tuple.is() is actually a useful thing. It is harnessed in
the tests a lot, and is likely to be already used by customers,
because it is available in box.tuple.* for a long time. It is a
matter of time when someone will open a doc ticket saying that
box.tuple.is() is not documented.

So the patch makes it legally public.

Follow-up #4684

@TarantoolBot document
Title: box.tuple.is()
A function to check whether a given object is a tuple cdata
object. Returns true or false. Never raises nor returns an error.
---
 src/box/lua/tuple.lua   |  7 ++++---
 test/box/tuple.result   | 40 ++++++++++++++++++++++++++++++++++++++++
 test/box/tuple.test.lua | 14 ++++++++++++++
 3 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
index eb3946a0f..f97aa1579 100644
--- a/src/box/lua/tuple.lua
+++ b/src/box/lua/tuple.lua
@@ -352,7 +352,8 @@ internal.tuple.tostring = nil
 internal.tuple.bless = tuple_bless
 internal.tuple.encode = tuple_encode
 
--- The function is internal in a sense that it is not documented.
--- But it is safe and widely used in the tests. Keep it here at
--- least for test code.
+-- Public API, additional to implemented in C.
+
+-- is() is implemented in Lua, because then it is
+-- easy to be JITed.
 box.tuple.is = is_tuple
diff --git a/test/box/tuple.result b/test/box/tuple.result
index 78f919deb..a499aa43a 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -1450,6 +1450,46 @@ level == max_depth + 5 or {level, max_depth}
 ---
 - true
 ...
+-- gh-4684: some box.tuple.* methods were private and could be
+-- used by customers to shoot in their own legs. Some of them
+-- were moved to a more secret place. box.tuple.is() was moved to
+-- the public API, legally.
+box.tuple.is()
+---
+- false
+...
+box.tuple.is(nil)
+---
+- false
+...
+box.tuple.is(box.NULL)
+---
+- false
+...
+box.tuple.is({})
+---
+- false
+...
+box.tuple.is(ffi.new('char[1]'))
+---
+- false
+...
+box.tuple.is(1)
+---
+- false
+...
+box.tuple.is('1')
+---
+- false
+...
+box.tuple.is(box.tuple.new())
+---
+- true
+...
+box.tuple.is(box.tuple.new({1}))
+---
+- true
+...
 msgpack.cfg({encode_max_depth = max_depth, encode_deep_as_nil = deep_as_nil})
 ---
 ...
diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
index baf2f22d5..b83fca5cd 100644
--- a/test/box/tuple.test.lua
+++ b/test/box/tuple.test.lua
@@ -496,4 +496,18 @@ while tuple ~= nil do level = level + 1 tuple = tuple[1] end
 -- serializer allows deeper tables.
 level == max_depth + 5 or {level, max_depth}
 
+-- gh-4684: some box.tuple.* methods were private and could be
+-- used by customers to shoot in their own legs. Some of them
+-- were moved to a more secret place. box.tuple.is() was moved to
+-- the public API, legally.
+box.tuple.is()
+box.tuple.is(nil)
+box.tuple.is(box.NULL)
+box.tuple.is({})
+box.tuple.is(ffi.new('char[1]'))
+box.tuple.is(1)
+box.tuple.is('1')
+box.tuple.is(box.tuple.new())
+box.tuple.is(box.tuple.new({1}))
+
 msgpack.cfg({encode_max_depth = max_depth, encode_deep_as_nil = deep_as_nil})
-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup
  2020-02-15 18:08 [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Vladislav Shpilevoy
  2020-02-15 18:08 ` [Tarantool-patches] [PATCH 1/2] tuple: hide internal functions from box.tuple.* Vladislav Shpilevoy
  2020-02-15 18:08 ` [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public Vladislav Shpilevoy
@ 2020-02-15 19:02 ` Oleg Babin
  2020-02-16 15:07 ` Vladislav Shpilevoy
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Oleg Babin @ 2020-02-15 19:02 UTC (permalink / raw)
  To: Vladislav Shpilevoy, tarantool-patches, alexander.turenko, imun

Hi! Thanks for your patch!

On 15/02/2020 21:08, Vladislav Shpilevoy wrote:
 > Branch: 
http://github.com/tarantool/tarantool/tree/gerold103/gh-4662-fiber-storage-leak
 > Issue: https://github.com/tarantool/tarantool/issues/4662
 >

I think you made a mistake and your branch is
https://github.com/tarantool/tarantool/compare/gerold103/gh-4684-tuple-bless-crash 
and issue is #4684

Also I've found that "next", "ipairs", "slice" and "upsert" are not 
documented as well.

Patches LGTM.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup
  2020-02-15 18:08 [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Vladislav Shpilevoy
                   ` (2 preceding siblings ...)
  2020-02-15 19:02 ` [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Oleg Babin
@ 2020-02-16 15:07 ` Vladislav Shpilevoy
  2020-03-01 14:21 ` Igor Munkin
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-16 15:07 UTC (permalink / raw)
  To: tarantool-patches, babinoleg, alexander.turenko, imun

I forgot ChangeLog:

@ChangeLog
- box.tuple.* namespace is cleaned up from private functions, and
  missing methods are documented (gh-4684).

And as Oleg pointed out, links to the issue and branch
are misleading. Here are correct links:

Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4684-tuple-bless-crash
Issue: https://github.com/tarantool/tarantool/issues/4684

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public
  2020-02-15 18:08 ` [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public Vladislav Shpilevoy
@ 2020-02-16 15:07   ` Vladislav Shpilevoy
  2020-02-17 20:21     ` Oleg Babin
  2020-03-16 13:19   ` Nikita Pettik
  1 sibling, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-16 15:07 UTC (permalink / raw)
  To: tarantool-patches, babinoleg, alexander.turenko, imun

As Oleg noticed, some other tuple methods were public, but
were not documented. I changed the commit message and doc
request to document them too.

================================================================================

tuple: document all box.tuple.* methods

In #4684 it was found that box.tuple.* contained some private
functions: bless(), encode(), and is().

Bless() and encode() didn't make any sense for a user, so they
were hidden into box.internal.tuple.*.

But box.tuple.is() is actually a useful thing. It is harnessed in
the tests a lot, and is likely to be already used by customers,
because it is available in box.tuple.* for a long time. It is a
matter of time when someone will open a doc ticket saying that
box.tuple.is() is not documented. The patch makes it legally
public.

Alongside it was discovered that tuple:next()/ipairs()/slice()/
upsert() were public, but were not documented. The patch carries a
docbot request for them all.

Follow-up #4684

@TarantoolBot document
Title: box.tuple.is()/next()/ipairs()/slice()/upsert()

```Lua
box.tuple.is(object)
```
A function to check whether a given object is a tuple cdata
object. Returns true or false. Never raises nor returns an error.

```Lua
tuple_object:next(pos)
```
An analogue of Lua `next()` function, but for a tuple object.
Although `tuple:next()` is not really efficient, and it is better
to use `tuple:pairs()/ipairs()`.
```
tarantool> t = box.tuple.new({1, 2, 3})
tarantool> ctx, field = t:next()
tarantool> while field do
    print(field)
    ctx, field = t:next(ctx)
end
tarantool>
1 2 3
```

```Lua
tuple_object:ipairs()
```
The same as `tuple_object:pairs()`. Because tuple fields are
integer always.

```Lua
tuple_object:slice(from[, to])
```
Extract tuple fields by a given range. `From` is not included. So
to take fields starting from the first use `from = 0`. `To` is
included and should be > `from`.
```
tarantool> t = box.tuple.new({1, 2, 3})
tarantool> t:slice(0)
---
- 1
- 2
- 3
...

tarantool> t:slice(0, 1)
---
- 1
...

tarantool> t:slice(1, 3)
---
- 2
- 3
...
```

```Lua
tuple_object:upsert()
```
The same as `tuple_object:update()`, but ignores errors. In case
of an error the tuple is left intact, but an error message is
printed. Only client errors are ignored, such as a bad field type,
or wrong field index/name. System errors, such as OOM, are not
ignored and raised just like with normal `update()`. Note, that
only bad operations are ignored. All correct operations are
applied.
```
tarantool> t = box.tuple.new({1, 2, 3})
tarantool> t2 = t:upsert({{'=', 5, 100}})
UPSERT operation failed:
ER_NO_SUCH_FIELD_NO: Field 5 was not found in the tuple
---
...

tarantool> t
---
- [1, 2, 3]
...

tarantool> t2
---
- [1, 2, 3]
...

tarantool> t2 = t:upsert({{'=', 5, 100}, {'+', 1, 3}})
UPSERT operation failed:
ER_NO_SUCH_FIELD_NO: Field 5 was not found in the tuple
---
...

tarantool> t
---
- [1, 2, 3]
...

tarantool> t2
---
- [4, 2, 3]
...
```
See how in the last example one operation is applied, and one is
not.

All methods of `tuple_object` are also available in `box.tuple.*`,
and a tuple needs to be passed explicitly then. Example below:
```
tarantool> t = box.tuple.new({1, 2, 3})
tarantool> box.tuple.slice(t, 0, 1)
---
- 1
...
```

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public
  2020-02-16 15:07   ` Vladislav Shpilevoy
@ 2020-02-17 20:21     ` Oleg Babin
  2020-02-17 21:11       ` Vladislav Shpilevoy
  0 siblings, 1 reply; 15+ messages in thread
From: Oleg Babin @ 2020-02-17 20:21 UTC (permalink / raw)
  To: tarantool-patches

HI! Vlad!

It seems that "slice" is redundant. I've filed an issue 
https://github.com/tarantool/doc/issues/1123. Alexandr T. found that 
"slise" was removed from doc in 1.6.5.

Quite strange why this method was not deleted later. But I think that it 
should be dropped from your commit message. And may be we should discuss 
depreciation and deletion os this method.

On 16/02/2020 18:07, Vladislav Shpilevoy wrote:
> As Oleg noticed, some other tuple methods were public, but
> were not documented. I changed the commit message and doc
> request to document them too.
> 
> ================================================================================
> 
> tuple: document all box.tuple.* methods
> 
> In #4684 it was found that box.tuple.* contained some private
> functions: bless(), encode(), and is().
> 
> Bless() and encode() didn't make any sense for a user, so they
> were hidden into box.internal.tuple.*.
> 
> But box.tuple.is() is actually a useful thing. It is harnessed in
> the tests a lot, and is likely to be already used by customers,
> because it is available in box.tuple.* for a long time. It is a
> matter of time when someone will open a doc ticket saying that
> box.tuple.is() is not documented. The patch makes it legally
> public.
> 
> Alongside it was discovered that tuple:next()/ipairs()/slice()/
> upsert() were public, but were not documented. The patch carries a
> docbot request for them all.
> 
> Follow-up #4684
> 
> @TarantoolBot document
> Title: box.tuple.is()/next()/ipairs()/slice()/upsert()
> 
> ```Lua
> box.tuple.is(object)
> ```
> A function to check whether a given object is a tuple cdata
> object. Returns true or false. Never raises nor returns an error.
> 
> ```Lua
> tuple_object:next(pos)
> ```
> An analogue of Lua `next()` function, but for a tuple object.
> Although `tuple:next()` is not really efficient, and it is better
> to use `tuple:pairs()/ipairs()`.
> ```
> tarantool> t = box.tuple.new({1, 2, 3})
> tarantool> ctx, field = t:next()
> tarantool> while field do
>      print(field)
>      ctx, field = t:next(ctx)
> end
> tarantool>
> 1 2 3
> ```
> 
> ```Lua
> tuple_object:ipairs()
> ```
> The same as `tuple_object:pairs()`. Because tuple fields are
> integer always.
> 
> ```Lua
> tuple_object:slice(from[, to])
> ```
> Extract tuple fields by a given range. `From` is not included. So
> to take fields starting from the first use `from = 0`. `To` is
> included and should be > `from`.
> ```
> tarantool> t = box.tuple.new({1, 2, 3})
> tarantool> t:slice(0)
> ---
> - 1
> - 2
> - 3
> ...
> 
> tarantool> t:slice(0, 1)
> ---
> - 1
> ...
> 
> tarantool> t:slice(1, 3)
> ---
> - 2
> - 3
> ...
> ```
> 
> ```Lua
> tuple_object:upsert()
> ```
> The same as `tuple_object:update()`, but ignores errors. In case
> of an error the tuple is left intact, but an error message is
> printed. Only client errors are ignored, such as a bad field type,
> or wrong field index/name. System errors, such as OOM, are not
> ignored and raised just like with normal `update()`. Note, that
> only bad operations are ignored. All correct operations are
> applied.
> ```
> tarantool> t = box.tuple.new({1, 2, 3})
> tarantool> t2 = t:upsert({{'=', 5, 100}})
> UPSERT operation failed:
> ER_NO_SUCH_FIELD_NO: Field 5 was not found in the tuple
> ---
> ...
> 
> tarantool> t
> ---
> - [1, 2, 3]
> ...
> 
> tarantool> t2
> ---
> - [1, 2, 3]
> ...
> 
> tarantool> t2 = t:upsert({{'=', 5, 100}, {'+', 1, 3}})
> UPSERT operation failed:
> ER_NO_SUCH_FIELD_NO: Field 5 was not found in the tuple
> ---
> ...
> 
> tarantool> t
> ---
> - [1, 2, 3]
> ...
> 
> tarantool> t2
> ---
> - [4, 2, 3]
> ...
> ```
> See how in the last example one operation is applied, and one is
> not.
> 
> All methods of `tuple_object` are also available in `box.tuple.*`,
> and a tuple needs to be passed explicitly then. Example below:
> ```
> tarantool> t = box.tuple.new({1, 2, 3})
> tarantool> box.tuple.slice(t, 0, 1)
> ---
> - 1
> ...
> ```
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public
  2020-02-17 20:21     ` Oleg Babin
@ 2020-02-17 21:11       ` Vladislav Shpilevoy
  0 siblings, 0 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-17 21:11 UTC (permalink / raw)
  To: Oleg Babin, tarantool-patches

Hi!

Ok, changed back to box.tuple.is() doc request.
New commit message:

	tuple: document all missed box.tuple.* methods

	In #4684 it was found that box.tuple.* contained some private
	functions: bless(), encode(), and is().

	Bless() and encode() didn't make any sense for a user, so they
	were hidden into box.internal.tuple.*.

	But box.tuple.is() is actually a useful thing. It is harnessed in
	the tests a lot, and is likely to be already used by customers,
	because it is available in box.tuple.* for a long time. It is a
	matter of time when someone will open a doc ticket saying that
	box.tuple.is() is not documented. The patch makes it legally
	public.

	Follow-up #4684

	@TarantoolBot document
	Title: box.tuple.is()
	```Lua
	box.tuple.is(object)
	```
	A function to check whether a given object is a tuple cdata
	object. Returns true or false. Never raises nor returns an error.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup
  2020-02-15 18:08 [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Vladislav Shpilevoy
                   ` (3 preceding siblings ...)
  2020-02-16 15:07 ` Vladislav Shpilevoy
@ 2020-03-01 14:21 ` Igor Munkin
  2020-03-15 17:42 ` Vladislav Shpilevoy
  2020-03-16 14:07 ` Kirill Yukhin
  6 siblings, 0 replies; 15+ messages in thread
From: Igor Munkin @ 2020-03-01 14:21 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: babinoleg, tarantool-patches

Vlad,

Thanks for the patch, LGTM.

On 15.02.20, Vladislav Shpilevoy wrote:
> The patchset removes or documents some internal functions in
> box.tuple.* namespace: box.tuple.bless()/encode()/is().
> 
> Bless() and encode() were moved to a more secret place, where
> suicidal users can't find it.
> 
> Is() is documented, because it is actually a useful thing.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4662-fiber-storage-leak
> Issue: https://github.com/tarantool/tarantool/issues/4662
> 
> Vladislav Shpilevoy (2):
>   tuple: hide internal functions from box.tuple.*
>   tuple: make box.tuple.is() public
> 
>  src/box/lua/schema.lua  |  4 ++--
>  src/box/lua/tuple.lua   | 15 +++++++++++++--
>  test/box/tuple.result   | 40 ++++++++++++++++++++++++++++++++++++++++
>  test/box/tuple.test.lua | 14 ++++++++++++++
>  4 files changed, 69 insertions(+), 4 deletions(-)
> 
> -- 
> 2.21.1 (Apple Git-122.3)
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup
  2020-02-15 18:08 [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Vladislav Shpilevoy
                   ` (4 preceding siblings ...)
  2020-03-01 14:21 ` Igor Munkin
@ 2020-03-15 17:42 ` Vladislav Shpilevoy
  2020-03-15 22:38   ` Nikita Pettik
  2020-03-16 14:07 ` Kirill Yukhin
  6 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-15 17:42 UTC (permalink / raw)
  To: tarantool-patches, babinoleg, imun, n.pettik

Nikita, could you please take a look? I don't know
who else to ask.

On 15/02/2020 19:08, Vladislav Shpilevoy wrote:
> The patchset removes or documents some internal functions in
> box.tuple.* namespace: box.tuple.bless()/encode()/is().
> 
> Bless() and encode() were moved to a more secret place, where
> suicidal users can't find it.
> 
> Is() is documented, because it is actually a useful thing.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4662-fiber-storage-leak
> Issue: https://github.com/tarantool/tarantool/issues/4662
> 
> Vladislav Shpilevoy (2):
>   tuple: hide internal functions from box.tuple.*
>   tuple: make box.tuple.is() public
> 
>  src/box/lua/schema.lua  |  4 ++--
>  src/box/lua/tuple.lua   | 15 +++++++++++++--
>  test/box/tuple.result   | 40 ++++++++++++++++++++++++++++++++++++++++
>  test/box/tuple.test.lua | 14 ++++++++++++++
>  4 files changed, 69 insertions(+), 4 deletions(-)
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup
  2020-03-15 17:42 ` Vladislav Shpilevoy
@ 2020-03-15 22:38   ` Nikita Pettik
  0 siblings, 0 replies; 15+ messages in thread
From: Nikita Pettik @ 2020-03-15 22:38 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: babinoleg, tarantool-patches

On 15 Mar 18:42, Vladislav Shpilevoy wrote:
> Nikita, could you please take a look? I don't know
> who else to ask.

Sure, tomorrow will do.

> On 15/02/2020 19:08, Vladislav Shpilevoy wrote:
> > The patchset removes or documents some internal functions in
> > box.tuple.* namespace: box.tuple.bless()/encode()/is().
> > 
> > Bless() and encode() were moved to a more secret place, where
> > suicidal users can't find it.
> > 
> > Is() is documented, because it is actually a useful thing.
> > 
> > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4662-fiber-storage-leak
> > Issue: https://github.com/tarantool/tarantool/issues/4662
> > 
> > Vladislav Shpilevoy (2):
> >   tuple: hide internal functions from box.tuple.*
> >   tuple: make box.tuple.is() public
> > 
> >  src/box/lua/schema.lua  |  4 ++--
> >  src/box/lua/tuple.lua   | 15 +++++++++++++--
> >  test/box/tuple.result   | 40 ++++++++++++++++++++++++++++++++++++++++
> >  test/box/tuple.test.lua | 14 ++++++++++++++
> >  4 files changed, 69 insertions(+), 4 deletions(-)
> > 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/2] tuple: hide internal functions from box.tuple.*
  2020-02-15 18:08 ` [Tarantool-patches] [PATCH 1/2] tuple: hide internal functions from box.tuple.* Vladislav Shpilevoy
@ 2020-03-16 13:15   ` Nikita Pettik
  0 siblings, 0 replies; 15+ messages in thread
From: Nikita Pettik @ 2020-03-16 13:15 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: babinoleg, tarantool-patches

On 15 Feb 19:08, Vladislav Shpilevoy wrote:
> box.tuple.bless, .encode, and .is are internal. Their
> behaviour is not documented, and they may omit some checks for the
> sake of speed, and can crash if used without thinking.
> 
> Nonetheless, despite they are not documented, curious users could
> notice them in box.tuple.* output via autocompletion, for example.
> And they could try to use them. This is not ok.
> 
> box.tuple.bless() being called by a user leads either to a crash,
> or to a leak (because it is basically tuple reference counter
> increment).
> 
> box.tuple.encode() is kind of a wrapper around msgpack, and users
> should not touch it. It may change, may be removed. And is just
> makes no sense except some rare cases in schema.lua.
> 
> bless() and encode() were used in schema.lua only, so the patch
> simply moves them to box.internal.tuple.
> 
> box.tuple.is() is kept as is, because
> - this is used in the tests a lot;
> - it is totally safe;
> - that function actually makes sense, and some users could have
>   already started using it.
> 
> There is no a test, since nothing to test - bless() is not
> available for users anymore (assuming no one is brave enough to
> rely on box.internal).

LGTM

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public
  2020-02-15 18:08 ` [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public Vladislav Shpilevoy
  2020-02-16 15:07   ` Vladislav Shpilevoy
@ 2020-03-16 13:19   ` Nikita Pettik
  1 sibling, 0 replies; 15+ messages in thread
From: Nikita Pettik @ 2020-03-16 13:19 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: babinoleg, tarantool-patches

On 15 Feb 19:08, Vladislav Shpilevoy wrote:
> In #4684 it was found that box.tuple.* contained some private
> functions: bless(), encode(), and is().
> 
> Bless() and encode() didn't make any sense for a user, so they
> were hidden into box.internal.tuple.*.
> 
> But box.tuple.is() is actually a useful thing. It is harnessed in
> the tests a lot, and is likely to be already used by customers,
> because it is available in box.tuple.* for a long time. It is a
> matter of time when someone will open a doc ticket saying that
> box.tuple.is() is not documented.
> 
> So the patch makes it legally public.
> 
> Follow-up #4684
> 
> @TarantoolBot document
> Title: box.tuple.is()
> A function to check whether a given object is a tuple cdata
> object. Returns true or false. Never raises nor returns an error.

LGTM

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup
  2020-02-15 18:08 [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Vladislav Shpilevoy
                   ` (5 preceding siblings ...)
  2020-03-15 17:42 ` Vladislav Shpilevoy
@ 2020-03-16 14:07 ` Kirill Yukhin
  2020-03-16 17:48   ` Nikita Pettik
  6 siblings, 1 reply; 15+ messages in thread
From: Kirill Yukhin @ 2020-03-16 14:07 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: babinoleg, tarantool-patches

Hello,

On 15 фев 19:08, Vladislav Shpilevoy wrote:
> The patchset removes or documents some internal functions in
> box.tuple.* namespace: box.tuple.bless()/encode()/is().
> 
> Bless() and encode() were moved to a more secret place, where
> suicidal users can't find it.
> 
> Is() is documented, because it is actually a useful thing.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4662-fiber-storage-leak
> Issue: https://github.com/tarantool/tarantool/issues/4662

The patch set LGTM.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup
  2020-03-16 14:07 ` Kirill Yukhin
@ 2020-03-16 17:48   ` Nikita Pettik
  0 siblings, 0 replies; 15+ messages in thread
From: Nikita Pettik @ 2020-03-16 17:48 UTC (permalink / raw)
  To: Kirill Yukhin; +Cc: babinoleg, tarantool-patches, Vladislav Shpilevoy

On 16 Mar 17:07, Kirill Yukhin wrote:
> Hello,
> 
> On 15 фев 19:08, Vladislav Shpilevoy wrote:
> > The patchset removes or documents some internal functions in
> > box.tuple.* namespace: box.tuple.bless()/encode()/is().
> > 
> > Bless() and encode() were moved to a more secret place, where
> > suicidal users can't find it.
> > 
> > Is() is documented, because it is actually a useful thing.
> > 
> > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4662-fiber-storage-leak
> > Issue: https://github.com/tarantool/tarantool/issues/4662
> 
> The patch set LGTM.

Pushed to master, 2.3 and 2.2. Changelog is updated correspondingly.
 
> --
> Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-03-16 17:48 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-15 18:08 [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Vladislav Shpilevoy
2020-02-15 18:08 ` [Tarantool-patches] [PATCH 1/2] tuple: hide internal functions from box.tuple.* Vladislav Shpilevoy
2020-03-16 13:15   ` Nikita Pettik
2020-02-15 18:08 ` [Tarantool-patches] [PATCH 2/2] tuple: make box.tuple.is() public Vladislav Shpilevoy
2020-02-16 15:07   ` Vladislav Shpilevoy
2020-02-17 20:21     ` Oleg Babin
2020-02-17 21:11       ` Vladislav Shpilevoy
2020-03-16 13:19   ` Nikita Pettik
2020-02-15 19:02 ` [Tarantool-patches] [PATCH 0/2] box.tuple.* cleanup Oleg Babin
2020-02-16 15:07 ` Vladislav Shpilevoy
2020-03-01 14:21 ` Igor Munkin
2020-03-15 17:42 ` Vladislav Shpilevoy
2020-03-15 22:38   ` Nikita Pettik
2020-03-16 14:07 ` Kirill Yukhin
2020-03-16 17:48   ` Nikita Pettik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox