Re: [Tarantool-patches] [PATCH v2 2/5] error: Add the custom error type

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

Sorry, some of my comments may contain typos, because it is
very late and I can't continue polishing it today.

See 21 comments below, review fixes in the end of the email,
and on a new branch in a separate commit.

On 10/04/2020 10:10, Leonid Vasiliev wrote:
> A possibility to create an error with a custom subtype was added.
> 
> In accordance with https://github.com/tarantool/tarantool/issues/4398
> a custom error type has been added to the box.error.
> Now, it's possible to create an error with a custom subtype (string value)
> for use it in applications.
> 
> @TarantoolBot document
> Title: error.custom_type

1. custom_type field does not exist in any user visible API. Even
after this patch.

> A custom error type has been added to the box.error.
> Now, it's possible to create an error with a custom subtype (string value)
> for use it in applications.

2. When docbot duplicates commit message, you can omit the latter.

> 
> To API has been added:
> - For create CustomError: box.error.new(type(str), format, ...)
>                           box.error.new({type = str,
>                                          reason = str})
> 
> - error.type - CustomType error return custom type
>     other errors return "base" error tybe
> - base_type - return "base" errror type
> Example:
> 
> err_custom = box.error.new("My Custom Type", "Reason")
> Now:
> err_custom.type == "My Custom Type"
> err_custom.base_type == "CustomType"
> err_custom.message == "Reason"

3. There is no a single word about name limit in 63 bytes.

> 
> Needed for #4398
> ---
>  src/box/errcode.h             |   1 +
>  src/box/error.cc              |  59 +++++++++++++++++
>  src/box/error.h               |  38 ++++++++++-
>  src/box/lua/error.cc          | 147 ++++++++++++++++++++++++++++++------------
>  src/lua/error.lua             |  11 ++++
>  test/app/fiber.result         |   5 +-
>  test/box/error.result         |  10 +--
>  test/engine/func_index.result |  18 +++---
>  8 files changed, 234 insertions(+), 55 deletions(-)
> 
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 233b312..8179e52 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -125,6 +125,31 @@ box_error_add(const char *file, unsigned line, uint32_t code,
>  
>  /* }}} */
>  
> +const char *
> +box_custom_error_type(const box_error_t *e)

4. box_error_t is a type for the public API. This API is not public and
therefore should use 'struct error' type name.

5. Naming policy for functions is that there should be a prefix
with the object type name in all functions. Or at least the prefix
should be the same on all methods of one type.

Here the prefix should be box_error_*, so this method is
box_error_custom_type(), and the method below is box_error_custom_new().

> +{
> +	CustomError *custom_error = type_cast(CustomError, e);
> +	if (custom_error)
> +		return custom_error->custom_type();
> +
> +	return NULL;
> +}
> +
> +struct error *
> +box_custom_error_new(const char *file, unsigned line,
> +		     const char *custom, const char *fmt, ...)
> +{
> +	struct error *e = BuildCustomError(file, line, custom);
> +	CustomError *custom_error = type_cast(CustomError, e);
> +	if (custom_error != NULL) {
> +		va_list ap;
> +		va_start(ap, fmt);
> +		error_vformat_msg(e, fmt, ap);
> +		va_end(ap);
> +	}
> +	return e;
> +}
> +
>  struct rmean *rmean_error = NULL;
>  
>  const char *rmean_error_strings[RMEAN_ERROR_LAST] = {
> @@ -290,3 +315,37 @@ BuildAccessDeniedError(const char *file, unsigned int line,
>  		return e;
>  	}
>  }
> +
> +static struct method_info customerror_methods[] = {
> +	make_method(&type_CustomError, "custom_type", &CustomError::custom_type),
> +	METHODS_SENTINEL
> +};
> +
> +const struct type_info type_CustomError =
> +	make_type("CustomError", &type_ClientError,
> +		  customerror_methods);

6. Make_type() perfectly fits in one line.

> +
> +CustomError::CustomError(const char *file, unsigned int line,
> +			 const char *custom_type)
> +	:ClientError(&type_CustomError, file, line, ER_CUSTOM_ERROR)
> +{
> +	error_format_msg(this, tnt_errcode_desc(m_errcode),
> +			 custom_type ?: "");

7. custom_type is never NULL.

> +
> +	if (custom_type) {
> +		strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
> +		m_custom_type[sizeof(m_custom_type) - 1] = '\0';
> +	} else {
> +		m_custom_type[0] = '\0';
> +	}
> +}
> +
> +struct error *
> +BuildCustomError(const char *file, unsigned int line, const char *custom_type)
> +{
> +	try {
> +		return new CustomError(file, line, custom_type);
> +	} catch (OutOfMemory *e) {
> +		return e;
> +	}
> +}
> diff --git a/src/box/error.h b/src/box/error.h
> index ca5d5b2..5013488 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -53,6 +53,9 @@ struct error *
>  BuildXlogGapError(const char *file, unsigned line,
>  		  const struct vclock *from, const struct vclock *to);
>  
> +struct error *
> +BuildCustomError(const char *file, unsigned int line, const char *custom_type);
> +
>  /** \cond public */
>  
>  struct error;
> @@ -138,6 +141,14 @@ box_error_set(const char *file, unsigned line, uint32_t code,
>  /** \endcond public */
>  
>  /**
> + * Return the error custom type,
> + * \param error

8. \param or @param takes two arguments - parameter name and its
description. You provided only name, and the name does not match
the actual parameter, which is 'e'.

> + * \return pointer to custom error type. On error, return NULL

9. Please, try to start sentences from a capital letter, and finish
them with a dot.

> + */
> +const char *
> +box_custom_error_type(const box_error_t *e);
> +
> +/**
>   * Add error to the diagnostic area. In contrast to box_error_set()
>   * it does not replace previous error being set, but rather link
>   * them into list.
> @@ -155,15 +166,24 @@ box_error_add(const char *file, unsigned line, uint32_t code,
>  
>  /**
>   * Construct error object without setting it in the diagnostics
> - * area. On the memory allocation fail returns NULL.
> + * area. On the memory allocation fail returns  OutOfMemory error.

10. Unneccessary diff.

>   */
>  struct error *
>  box_error_new(const char *file, unsigned line, uint32_t code,
>  	      const char *fmt, ...);
>  
> +/**
> + * Construct error object without setting it in the diagnostics
> + * area. On the memory allocation fail returns OutOfMemory error.

11. This is a copy-paste box box_error_new() comment. Better write
here a couple of words how is it different. Or better merge box_error_new()
and box_custom_error_new(). Because in Lua I can specify both code
and type. Don't see why can't I do this in C.

Honestly, I think we should use kind of struct error_args like you
tried to do for lua/error.cc for the C API. Because with more
arguments it will be hard to maintain them all as function paramters.
But not now.

> + */
> +struct error *
> +box_custom_error_new(const char *file, unsigned line,
> +		     const char *custom, const char *fmt, ...);
> +
>  extern const struct type_info type_ClientError;
>  extern const struct type_info type_XlogError;
>  extern const struct type_info type_AccessDeniedError;
> +extern const struct type_info type_CustomError;
>  
>  #if defined(__cplusplus)
>  } /* extern "C" */
> @@ -290,6 +310,22 @@ struct XlogGapError: public XlogError
>  	virtual void raise() { throw this; }
>  };
>  
> +class CustomError: public ClientError
> +{
> +public:
> +	CustomError(const char *file, unsigned int line,
> +		    const char *custom_type);
> +
> +	const char*
> +	custom_type()
> +	{
> +		return m_custom_type;
> +	}
> +private:
> +	/** Custom type name */
> +	char m_custom_type[64];
> +};
> +
>  #endif /* defined(__cplusplus) */
>  
>  #endif /* TARANTOOL_BOX_ERROR_H_INCLUDED */
> diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
> index 4432bc8..1bcce1f 100644
> --- a/src/box/lua/error.cc
> +++ b/src/box/lua/error.cc
> @@ -42,64 +42,101 @@ extern "C" {
>  #include "lua/utils.h"
>  #include "box/error.h"
>  
> -/**
> - * Parse Lua arguments (they can come as single table
> - * f({code : number, reason : string}) or as separate members
> - * f(code, reason)) and construct struct error with given values.
> - * In case one of arguments is missing its corresponding field
> - * in struct error is filled with default value.
> - */
> -static struct error *

12. Most of changes in this file does not really realate to the
feature you are trying to introduce. It is just refactoring. If
you want it, it should be done in a separate commit, because
otherwise it is hard to see which changes are functional and need
more attention.

> -luaT_error_create(lua_State *L, int top_base)
> +struct error_args {
> +	uint32_t code;
> +	const char *reason;
> +	const char *custom;
> +	bool tb_mode;
> +	bool tb_parsed;

13. All structures and their attributes usually have a comment
explaining their purpose. In this structure it is hard to
understand what is 'tb', what is 'tb_mode', what is 'tb_parsed',
if you are not in the context of the patch.

> +};
> +
> +static int
> +luaT_error_parse_args(struct lua_State *L, int top_base,
> +		      struct error_args *args)
>  {
> -	uint32_t code = 0;
> -	const char *reason = NULL;
> -	bool tb_parsed = false;
> -	bool tb_mode = false;
> -	const char *file = "";
> -	unsigned line = 0;
> -	lua_Debug info;
>  	int top = lua_gettop(L);
> -	if (top >= top_base && lua_type(L, top_base) == LUA_TNUMBER) {
> -		code = lua_tonumber(L, top_base);
> -		reason = tnt_errcode_desc(code);
> +	int top_type = lua_type(L, top_base);
> +	if (top >= top_base &&
> +	    (top_type == LUA_TNUMBER || top_type == LUA_TSTRING)) {
> +		if (top_type == LUA_TNUMBER) {
> +			args->code = lua_tonumber(L, top_base);
> +		} else if (top_type == LUA_TSTRING) {
> +			args->code = ER_CUSTOM_ERROR;
> +			args->custom = lua_tostring(L, top_base);
> +		} else {
> +			return -1;
> +		}
> +		args->reason = tnt_errcode_desc(args->code);
>  		if (top > top_base) {
>  			/* Call string.format(reason, ...) to format message */
>  			lua_getglobal(L, "string");
>  			if (lua_isnil(L, -1))
> -				goto raise;
> +				return 0;
>  			lua_getfield(L, -1, "format");
>  			if (lua_isnil(L, -1))
> -				goto raise;
> -			lua_pushstring(L, reason);
> +				return 0;
> +			lua_pushstring(L, args->reason);
>  			for (int i = top_base + 1; i <= top; i++)
>  				lua_pushvalue(L, i);
>  			lua_call(L, top - top_base + 1, 1);
> -			reason = lua_tostring(L, -1);
> -		} else if (strchr(reason, '%') != NULL) {
> +			args->reason = lua_tostring(L, -1);
> +		} else if (strchr(args->reason, '%') != NULL) {
>  			/* Missing arguments to format string */
> -			return NULL;
> +			return -1;
>  		}
> -	} else if (top == top_base && lua_istable(L, top_base)) {
> +	} else if (top == top_base && top_type == LUA_TTABLE) {
>  		lua_getfield(L, top_base, "code");
> -		code = lua_tonumber(L, -1);
> +		if (lua_isnil(L, -1) == 0)
> +			args->code = lua_tonumber(L, -1);

14. I reverted this change and didn't get any problems
in the tests. Why?

>  		lua_pop(L, 1);
>  		lua_getfield(L, top_base, "reason");
> -		reason = lua_tostring(L, -1);
> -		if (reason == NULL)
> -			reason = "";
> +		if (lua_isnil(L, -1) == 0)
> +			args->reason = lua_tostring(L, -1);

15. Previously it was not checked for nil, and all worked fine.
Why did you change that?

> +		lua_pop(L, 1);
> +		lua_getfield(L, top_base, "type");
> +		if (lua_isnil(L, -1) == 0)
> +			args->custom = lua_tostring(L, -1);
>  		lua_pop(L, 1);
>  		lua_getfield(L, top_base, "traceback");
>  		if (lua_isboolean(L, -1)) {
> -			tb_parsed = true;
> -			tb_mode = lua_toboolean(L, -1);
> +			args->tb_parsed = true;
> +			args->tb_mode = lua_toboolean(L, -1);
>  		}
>  		lua_pop(L, -1);
>  	} else {
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * Parse Lua arguments (they can come as single table
> + * f({code : number, reason : string}) or as separate members
> + * f(code, reason)) and construct struct error with given values.

16. The comment does not describe new syntax with 'type' argument.

> + * In case one of arguments is missing its corresponding field
> + * in struct error is filled with default value.
> + */
> +static struct error *
> +luaT_error_create(lua_State *L, int top_base)
> +{
> +	struct error_args args;
> +	args.code = UINT32_MAX;

17. Before your patch default error code was 0. Why is it
changed to UINT32_MAX?

> +	args.reason = NULL;
> +	args.custom = NULL;
> +	args.tb_mode = false;
> +	args.tb_parsed = false;
> +
> +	if (luaT_error_parse_args(L, top_base, &args) != 0) {
>  		return NULL;
>  	}
>  
> -raise:
> +	if (args.reason == NULL)
> +		args.reason = "";
> +
> +	const char *file = "";
> +	unsigned line = 0;
> +	lua_Debug info;
>  	if (lua_getstack(L, 1, &info) && lua_getinfo(L, "Sl", &info)) {
>  		if (*info.short_src) {
>  			file = info.short_src;
> @@ -111,9 +148,16 @@ raise:
>  		line = info.currentline;
>  	}
>  
> -	struct error *err = box_error_new(file, line, code, "%s", reason);
> -	if (tb_parsed)
> -		err->traceback_mode = tb_mode;
> +	struct error *err = NULL;
> +	if (args.custom) {
> +		err = box_custom_error_new(file, line, args.custom,
> +					   "%s", args.reason);

18. Code argument is ignored here. Although I am not sure it shouldn't
be ignored. From what I remember, we decided that code should stay,
but I may be wrong.

> +	} else {
> +		err = box_error_new(file, line, args.code, "%s", args.reason);
> +	}
> +
> +	if (args.tb_parsed)
> +		err->traceback_mode = args.tb_mode;
>  
>  	return err;
>  }
> @@ -162,17 +206,36 @@ luaT_error_last(lua_State *L)
>  static int
>  luaT_error_new(lua_State *L)
>  {
> -	if (lua_gettop(L) == 0)
> -		return luaL_error(L, "Usage: box.error.new(code, args)");
> +	if (lua_gettop(L) == 0) {
> +		return luaL_error(L, "Usage: box.error.new(code, args) or "\
> +				     "box.error.new(type, args)");
> +	}
>  	struct error *e = luaT_error_create(L, 1);
> -	if (e == NULL)
> -		return luaL_error(L, "Usage: box.error.new(code, args)");
> +	if (e == NULL) {
> +		return luaL_error(L, "Usage: box.error.new(code, args) or "\
> +				     "box.error.new(type, args)");
> +	}
>  	lua_settop(L, 0);
>  	luaT_pusherror(L, e);
>  	return 1;
>  }
>  
>  static int
> +luaT_error_custom_type(lua_State *L)

19. This method is not needed. We don't have 'global' getters
like box.error.prev, or box.error.type, and so on. Every
error object provides API to get its members without calling
global functions.

> +{
> +	struct error *e = luaL_checkerror(L, -1);
> +
> +	const char *custom_type = box_custom_error_type(e);
> +	if (custom_type == NULL) {
> +		lua_pushfstring(L, "The error has't custom type");
> +		return 1;
> +	}
> +
> +	lua_pushstring(L, custom_type);
> +	return 1;
> +}
> +
> +static int
>  luaT_error_clear(lua_State *L)
>  {
>  	if (lua_gettop(L) >= 1)
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 46d2866..5b32607 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -78,6 +78,16 @@ local function reflection_get(err, method)
>  end
>  
>  local function error_type(err)
> +    local res
> +    if ffi.string(err._type.name) == 'CustomError' then
> +        res = box.error.custom_type(err)
> +    else
> +        res = ffi.string(err._type.name)

20. ffi.string() is called on one string 2 times. But it
is not free, it allocates a new string each time.

> +    end
> +    return res
> +end

21. There is again not a single test.

Consider my review fixes below and on a new branch in a
separate commit. Note, it is not entirely separated
from the original commit in a couple of places, since I had
rebase conflicts.

Branch is:

    lvasiliev/gh-4398-expose-error-module-4-review

====================
    Review fixes
    
    New commit message proposal:
    
    error: add custom error type
    
    Part of #4398
    
    @TarantoolBot document
    Title: Custom error types for Lua errors
    
    Errors can be created in 2 ways: `box.error.new()` and `box.error()`.
    
    Both used to take either `code, reason, <reason string args>` or
    `{code = code, reason = reason, ...}` arguments.
    
    Now in the first option instead of code a user can specify a
    string as its own error type. In the second option a user can
    specify both code and type. For example:
    
    ```Lua
    box.error('MyErrorType', 'Message')
    box.error({type = 'MyErrorType', code = 1024, reason = 'Message'})
    ```
    Or no-throw version:
    ```Lua
    box.error.new('MyErrorType', 'Message')
    box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
    ```
    When a custom type is specified, it is shown in `err.type`
    attribute. When it is not specified, `err.type` shows one of
    built-in errors such as 'ClientError', 'OurOfMemory', etc.
    
    Name length limit on the custom type is 63 bytes. All is longer
    is truncated.
    
    Original error type can be checked using `err.base_type` member,
    although normally it should not be used. For user-defined types
    base type is 'CustomError'.
    
    For example:
    ```
    tarantool> e = box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
    ---
    ...
    
    tarantool> e:unpack()
    ---
    - code: 1024
      trace:
      - file: '[string "e = box.error.new({type = ''MyErrorType'', code..."]'
        line: 1
      type: MyErrorType
      custom_type: MyErrorType
      message: Message
      base_type: CustomError
    ...
    ```

diff --git a/extra/exports b/extra/exports
index ffd11145d..a5ebe0884 100644
--- a/extra/exports
+++ b/extra/exports
@@ -234,6 +234,7 @@ box_index_min
 box_index_max
 box_index_count
 box_error_type
+box_error_custom_type
 box_error_code
 box_error_message
 box_error_last
diff --git a/src/box/error.cc b/src/box/error.cc
index 8179e5287..d854b86e9 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -86,35 +86,51 @@ box_error_set(const char *file, unsigned line, uint32_t code,
 	return -1;
 }
 
+static struct error *
+box_error_new_va(const char *file, unsigned line, uint32_t code,
+		 const char *custom_type, const char *fmt, va_list ap)
+{
+	if (custom_type == NULL) {
+		struct error *e = BuildClientError(file, line, ER_UNKNOWN);
+		ClientError *client_error = type_cast(ClientError, e);
+		if (client_error != NULL) {
+			client_error->m_errcode = code;
+			error_vformat_msg(e, fmt, ap);
+		}
+		return e;
+	} else {
+		struct error *e = BuildCustomError(file, line, custom_type);
+		CustomError *custom_error = type_cast(CustomError, e);
+		if (custom_error != NULL) {
+			custom_error->m_errcode = code;
+			error_vformat_msg(e, fmt, ap);
+		}
+		return e;
+	}
+}
+
 struct error *
 box_error_new(const char *file, unsigned line, uint32_t code,
-	      const char *fmt, ...)
+	      const char *custom_type, const char *fmt, ...)
 {
-	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
-	ClientError *client_error = type_cast(ClientError, e);
-	if (client_error != NULL) {
-		client_error->m_errcode = code;
-		va_list ap;
-		va_start(ap, fmt);
-		error_vformat_msg(e, fmt, ap);
-		va_end(ap);
-	}
+	va_list ap;
+	va_start(ap, fmt);
+	struct error *e = box_error_new_va(file, line, code, custom_type,
+					   fmt, ap);
+	va_end(ap);
 	return e;
 }
 
 int
 box_error_add(const char *file, unsigned line, uint32_t code,
-	      const char *fmt, ...)
+	      const char *custom_type, const char *fmt, ...)
 {
-	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
-	ClientError *client_error = type_cast(ClientError, e);
-	if (client_error) {
-		client_error->m_errcode = code;
-		va_list ap;
-		va_start(ap, fmt);
-		error_vformat_msg(e, fmt, ap);
-		va_end(ap);
-	}
+	va_list ap;
+	va_start(ap, fmt);
+	struct error *e = box_error_new_va(file, line, code, custom_type,
+					   fmt, ap);
+	va_end(ap);
+
 	struct diag *d = &fiber()->diag;
 	if (diag_is_empty(d))
 		diag_set_error(d, e);
@@ -126,7 +142,7 @@ box_error_add(const char *file, unsigned line, uint32_t code,
 /* }}} */
 
 const char *
-box_custom_error_type(const box_error_t *e)
+box_error_custom_type(const struct error *e)
 {
 	CustomError *custom_error = type_cast(CustomError, e);
 	if (custom_error)
@@ -135,21 +151,6 @@ box_custom_error_type(const box_error_t *e)
 	return NULL;
 }
 
-struct error *
-box_custom_error_new(const char *file, unsigned line,
-		     const char *custom, const char *fmt, ...)
-{
-	struct error *e = BuildCustomError(file, line, custom);
-	CustomError *custom_error = type_cast(CustomError, e);
-	if (custom_error != NULL) {
-		va_list ap;
-		va_start(ap, fmt);
-		error_vformat_msg(e, fmt, ap);
-		va_end(ap);
-	}
-	return e;
-}
-
 struct rmean *rmean_error = NULL;
 
 const char *rmean_error_strings[RMEAN_ERROR_LAST] = {
@@ -322,22 +323,15 @@ static struct method_info customerror_methods[] = {
 };
 
 const struct type_info type_CustomError =
-	make_type("CustomError", &type_ClientError,
-		  customerror_methods);
+	make_type("CustomError", &type_ClientError, customerror_methods);
 
 CustomError::CustomError(const char *file, unsigned int line,
 			 const char *custom_type)
 	:ClientError(&type_CustomError, file, line, ER_CUSTOM_ERROR)
 {
-	error_format_msg(this, tnt_errcode_desc(m_errcode),
-			 custom_type ?: "");
-
-	if (custom_type) {
-		strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
-		m_custom_type[sizeof(m_custom_type) - 1] = '\0';
-	} else {
-		m_custom_type[0] = '\0';
-	}
+	error_format_msg(this, tnt_errcode_desc(m_errcode), custom_type);
+	strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
+	m_custom_type[sizeof(m_custom_type) - 1] = '\0';
 }
 
 struct error *
diff --git a/src/box/error.h b/src/box/error.h
index 501348885..540008f35 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -142,11 +142,11 @@ box_error_set(const char *file, unsigned line, uint32_t code,
 
 /**
  * Return the error custom type,
- * \param error
- * \return pointer to custom error type. On error, return NULL
+ * \param e Error object.
+ * \return Pointer to custom error type. On error return NULL.
  */
 const char *
-box_custom_error_type(const box_error_t *e);
+box_error_custom_type(const struct error *e);
 
 /**
  * Add error to the diagnostic area. In contrast to box_error_set()
@@ -154,6 +154,8 @@ box_custom_error_type(const box_error_t *e);
  * them into list.
  *
  * \param code IPROTO error code (enum \link box_error_code \endlink)
+ * \param custom_type User-defined error type which will be
+ *       displayed instead of ClientError.
  * \param format (const char * ) - printf()-like format string
  * \param ... - format arguments
  * \returns -1 for convention use
@@ -162,23 +164,15 @@ box_custom_error_type(const box_error_t *e);
  */
 int
 box_error_add(const char *file, unsigned line, uint32_t code,
-	      const char *fmt, ...);
+	      const char *custom_type, const char *fmt, ...);
 
 /**
  * Construct error object without setting it in the diagnostics
- * area. On the memory allocation fail returns  OutOfMemory error.
+ * area. On the memory allocation fail returns NULL.
  */
 struct error *
 box_error_new(const char *file, unsigned line, uint32_t code,
-	      const char *fmt, ...);
-
-/**
- * Construct error object without setting it in the diagnostics
- * area. On the memory allocation fail returns OutOfMemory error.
- */
-struct error *
-box_custom_error_new(const char *file, unsigned line,
-		     const char *custom, const char *fmt, ...);
+	      const char *custom_type, const char *fmt, ...);
 
 extern const struct type_info type_ClientError;
 extern const struct type_info type_XlogError;
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index dd7068a83..cf771935e 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -42,95 +42,83 @@ extern "C" {
 #include "lua/utils.h"
 #include "box/error.h"
 
-struct error_args {
-	uint32_t code;
-	const char *reason;
-	const char *custom;
-	const char *traceback;
-};
-
-static int
-luaT_error_parse_args(struct lua_State *L, int top_base,
-		      struct error_args *args)
+/**
+ * Parse Lua arguments (they can come as single table
+ * f({code : number, reason : string}) or as separate members
+ * f(code, reason)) and construct struct error with given values.
+ *
+ * Instead of 'code' it is possible to specify a string name of
+ * the error object's type:
+ *
+ *     box.error(type, reason, ...)
+ *     box.error({type = string, reason = string, ...})
+ *
+ * In case one of arguments is missing its corresponding field
+ * in struct error is filled with default value.
+ */
+static struct error *
+luaT_error_create(lua_State *L, int top_base)
 {
+	uint32_t code = 0;
+	const char *custom_type = NULL;
+	const char *reason = NULL;
+	bool is_traceback_enabled = false;
+	bool is_traceback_specified = false;
+	const char *file = "";
+	unsigned line = 0;
+	lua_Debug info;
 	int top = lua_gettop(L);
 	int top_type = lua_type(L, top_base);
-	if (top >= top_base &&
-	    (top_type == LUA_TNUMBER || top_type == LUA_TSTRING)) {
+	if (top >= top_base && (top_type == LUA_TNUMBER ||
+				top_type == LUA_TSTRING)) {
 		if (top_type == LUA_TNUMBER) {
-			args->code = lua_tonumber(L, top_base);
-		} else if (top_type == LUA_TSTRING) {
-			args->code = ER_CUSTOM_ERROR;
-			args->custom = lua_tostring(L, top_base);
+			code = lua_tonumber(L, top_base);
 		} else {
-			return -1;
+			code = ER_CUSTOM_ERROR;
+			custom_type = lua_tostring(L, top_base);
 		}
-		args->reason = tnt_errcode_desc(args->code);
+		reason = tnt_errcode_desc(code);
 		if (top > top_base) {
 			/* Call string.format(reason, ...) to format message */
 			lua_getglobal(L, "string");
 			if (lua_isnil(L, -1))
-				return 0;
+				goto raise;
 			lua_getfield(L, -1, "format");
 			if (lua_isnil(L, -1))
-				return 0;
-			lua_pushstring(L, args->reason);
+				goto raise;
+			lua_pushstring(L, reason);
 			for (int i = top_base + 1; i <= top; i++)
 				lua_pushvalue(L, i);
 			lua_call(L, top - top_base + 1, 1);
-			args->reason = lua_tostring(L, -1);
-		} else if (strchr(args->reason, '%') != NULL) {
+			reason = lua_tostring(L, -1);
+		} else if (strchr(reason, '%') != NULL) {
 			/* Missing arguments to format string */
-			return -1;
+			return NULL;
 		}
 	} else if (top == top_base && top_type == LUA_TTABLE) {
 		lua_getfield(L, top_base, "code");
-		if (lua_isnil(L, -1) == 0)
-			args->code = lua_tonumber(L, -1);
+		if (!lua_isnil(L, -1))
+			code = lua_tonumber(L, -1);
+		else
+			code = ER_CUSTOM_ERROR;
 		lua_getfield(L, top_base, "reason");
-		if (lua_isnil(L, -1) == 0)
-			args->reason = lua_tostring(L, -1);
+		reason = lua_tostring(L, -1);
+		if (reason == NULL)
+			reason = "";
 		lua_getfield(L, top_base, "type");
-		if (lua_isnil(L, -1) == 0)
-			args->custom = lua_tostring(L, -1);
+		if (!lua_isnil(L, -1))
+			custom_type = lua_tostring(L, -1);
 		lua_getfield(L, top_base, "traceback");
-		if (lua_isboolean(L, -1) && lua_toboolean(L, -1)) {
-			luaL_traceback(L, L, NULL, 0);
-			if (lua_isstring(L, -1))
-				args->traceback = lua_tostring(L, -1);
+		if (lua_isboolean(L, -1)) {
+			is_traceback_enabled = lua_toboolean(L, -1);
+			is_traceback_specified = true;
 		}
 		lua_pop(L, 1);
 	} else {
-		return -1;
-	}
-	return 0;
-}
-
-/**
- * Parse Lua arguments (they can come as single table
- * f({code : number, reason : string}) or as separate members
- * f(code, reason)) and construct struct error with given values.
- * In case one of arguments is missing its corresponding field
- * in struct error is filled with default value.
- */
-static struct error *
-luaT_error_create(lua_State *L, int top_base)
-{
-	struct error_args args;
-	args.code = UINT32_MAX;
-	args.reason = NULL;
-	args.custom = NULL;
-	args.traceback = NULL;
-
-	if (luaT_error_parse_args(L, top_base, &args) != 0)
 		return NULL;
+	}
 
-	if (args.reason == NULL)
-		args.reason = "";
-
-	const char *file = "";
-	unsigned line = 0;
-	lua_Debug info;
+raise:
 	if (lua_getstack(L, 1, &info) && lua_getinfo(L, "Sl", &info)) {
 		if (*info.short_src) {
 			file = info.short_src;
@@ -142,15 +130,13 @@ luaT_error_create(lua_State *L, int top_base)
 		line = info.currentline;
 	}
 
-	struct error *err;
-	if (args.custom) {
-		err = box_custom_error_new(file, line, args.custom,
-					   "%s", args.reason);
-	} else {
-		err = box_error_new(file, line, args.code, "%s", args.reason);
-	}
-	if (args.traceback != NULL && err != NULL)
-		error_set_traceback(err, args.traceback);
+	struct error *err = box_error_new(file, line, code, custom_type,
+					  "%s", reason);
+	/*
+	 * Explicit traceback option overrides the global setting.
+	 */
+	if (err != NULL && is_traceback_specified)
+		err->is_traceback_enabled = is_traceback_enabled;
 	return err;
 }
 
@@ -198,35 +184,16 @@ luaT_error_last(lua_State *L)
 static int
 luaT_error_new(lua_State *L)
 {
-	if (lua_gettop(L) == 0) {
-		return luaL_error(L, "Usage: box.error.new(code, args) or "\
-				     "box.error.new(type, args)");
-	}
-	struct error *e = luaT_error_create(L, 1);
-	if (e == NULL) {
+	struct error *e;
+	if (lua_gettop(L) == 0 || (e = luaT_error_create(L, 1)) == NULL) {
 		return luaL_error(L, "Usage: box.error.new(code, args) or "\
-				     "box.error.new(type, args)");
+				  "box.error.new(type, args)");
 	}
 	lua_settop(L, 0);
 	luaT_pusherror(L, e);
 	return 1;
 }
 
-static int
-luaT_error_custom_type(lua_State *L)
-{
-	struct error *e = luaL_checkerror(L, -1);
-
-	const char *custom_type = box_custom_error_type(e);
-	if (custom_type == NULL) {
-		lua_pushfstring(L, "The error has't custom type");
-		return 1;
-	}
-
-	lua_pushstring(L, custom_type);
-	return 1;
-}
-
 static int
 luaT_error_clear(lua_State *L)
 {
@@ -381,10 +348,6 @@ box_lua_error_init(struct lua_State *L) {
 			lua_pushcfunction(L, luaT_error_cfg);
 			lua_setfield(L, -2, "cfg");
 		}
-		{
-			lua_pushcfunction(L, luaT_error_custom_type);
-			lua_setfield(L, -2, "custom_type");
-		}
 		lua_setfield(L, -2, "__index");
 	}
 	lua_setmetatable(L, -2);
diff --git a/src/box/xrow.c b/src/box/xrow.c
index a494d1f46..5494b41cd 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1123,7 +1123,7 @@ iproto_decode_error_stack(const char **pos)
 				continue;
 			}
 		}
-		box_error_add(__FILE__, __LINE__, code, reason);
+		box_error_add(__FILE__, __LINE__, code, NULL, reason);
 	}
 	return 0;
 }
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 1f9ae9375..1d3413b19 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -37,6 +37,9 @@ exception_get_int(struct error *e, const struct method_info *method);
 
 int
 error_set_prev(struct error *e, struct error *prev);
+
+const char *
+box_error_custom_type(const struct error *e);
 ]]
 
 local REFLECTION_CACHE = {}
@@ -77,20 +80,18 @@ local function reflection_get(err, method)
     end
 end
 
-local function error_type(err)
-    local res
-    if ffi.string(err._type.name) == 'CustomError' then
-        res = box.error.custom_type(err)
-    else
-        res = ffi.string(err._type.name)
-    end
-    return res
-end
-
 local function error_base_type(err)
     return ffi.string(err._type.name)
 end
 
+local function error_type(err)
+    local res = ffi.C.box_error_custom_type(err)
+    if res ~= nil then
+        return ffi.string(res)
+    end
+    return error_base_type(err)
+end
+
 local function error_message(err)
     return ffi.string(err._errmsg)
 end
@@ -148,7 +149,7 @@ local error_fields = {
     ["errno"]       = error_errno;
     ["prev"]        = error_prev;
     ["traceback"]   = error_traceback;
-    ["base_type"]    = error_base_type
+    ["base_type"]   = error_base_type,
 }
 
 local function error_unpack(err)
diff --git a/test/box/error.result b/test/box/error.result
index 2b837e434..b717a4ff4 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -431,11 +431,8 @@ t;
  |   210: box.error.SQL_PREPARE
  |   211: box.error.WRONG_QUERY_ID
  |   212: box.error.SEQUENCE_NOT_STARTED
-<<<<<<< HEAD
  |   213: box.error.NO_SUCH_SESSION_SETTING
-=======
- |   213: box.error.CUSTOM_ERROR
->>>>>>> error: Add the custom error type
+ |   214: box.error.CUSTOM_ERROR
  | ...
 
 test_run:cmd("setopt delimiter ''");
@@ -895,3 +892,60 @@ check_trace(e:unpack().traceback)
  | ---
  | - true
  | ...
+
+--
+-- gh-4398: custom error type.
+--
+-- Try no code.
+e = box.error.new({type = 'TestType', reason = 'Test reason'})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 214
+ |   trace:
+ |   - file: '[string "e = box.error.new({type = ''TestType'', reason ..."]'
+ |     line: 1
+ |   type: TestType
+ |   custom_type: TestType
+ |   message: Test reason
+ |   base_type: CustomError
+ | ...
+-- Try code not the same as used by default.
+e = box.error.new({type = 'TestType', reason = 'Test reason', code = 123})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 123
+ |   trace:
+ |   - file: '[string "e = box.error.new({type = ''TestType'', reason ..."]'
+ |     line: 1
+ |   type: TestType
+ |   custom_type: TestType
+ |   message: Test reason
+ |   base_type: CustomError
+ | ...
+-- Try to omit message.
+e = box.error.new({type = 'TestType'})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 214
+ |   trace:
+ |   - file: '[string "e = box.error.new({type = ''TestType''}) "]'
+ |     line: 1
+ |   type: TestType
+ |   custom_type: TestType
+ |   message: 
+ |   base_type: CustomError
+ | ...
+-- Try too long type name.
+e = box.error.new({type = string.rep('a', 128)})
+ | ---
+ | ...
+#e.type
+ | ---
+ | - 63
+ | ...
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index 6f1271630..fe4051899 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -262,3 +262,19 @@ check_trace(t3(nil, false):unpack().traceback)
 box.error.cfg{traceback_enable = false}
 _, e = pcall(t3, true, true)
 check_trace(e:unpack().traceback)
+
+--
+-- gh-4398: custom error type.
+--
+-- Try no code.
+e = box.error.new({type = 'TestType', reason = 'Test reason'})
+e:unpack()
+-- Try code not the same as used by default.
+e = box.error.new({type = 'TestType', reason = 'Test reason', code = 123})
+e:unpack()
+-- Try to omit message.
+e = box.error.new({type = 'TestType'})
+e:unpack()
+-- Try too long type name.
+e = box.error.new({type = string.rep('a', 128)})
+#e.type