Re: [Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding

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

Thanks for the patch!

One major note: mpstream_iproto_encode_error() is not
updated at all. So the marshaling is still not complete.

Generally the patch is relatively fine. Still some
comments exist, but almost no major ones, except the
one above.

On 16/04/2020 19:38, Leonid Vasiliev wrote:
> Closes #4398
> 
> @TarantoolBot document
> Title: msgpack format for MP_ERROR
> 
> For sent an error over IProto MP_ERROR(0x03) subtype
> has been added to MsgPack MP_EXT.
> Now, if session setting 'error_marshaling_enabled'
> is true error will be encoded as:
>     ```
>         +--------+----------+========+
>         | MP_EXT | MP_ERROR | MP_MAP |
>         +--------+----------+========+
>     ```
> At the map passes all necessary parameters (depending
> from the error type) to create a copy of the error
> on the client side.
> The necessary fields:
> - Error type
> - Trace(file name and line)

Since we decided to drop traceback for now, we should omit
this too, until it is designed properly. Better not to add them
now and add later with a thorough design, than add in a hurry
now, and then not be able to change it.

> - Error message
> - Errno

You need to document the exact numbers and types of every
of these fields.

> Additional fields:
> - ClientError code
> - Traceback (now only Lua traceback)

Traceback is not here anymore.

> - Custom type
> - Access Denied object type
> - Access Denied object name
> - Access Denied access type

Your error structure is flat, and is based entirely on number
keys. Probably a better option would be to store all type-specific
optional attributes with MP_STR keys and in a separate map.

Separate map would make it clear what options are type-specific,
and would simplify decoding on the client a bit.

Talking of numbers, there is the problem of introduction of new
attributes and of compatibility again. About compatibility all
my arguments are the same as for error types. If you will use
string keys for type-specific and potentially extendible fields,
it will simplify support in connectors. About problem of new
attributes introduction see example below:

Assume, we have AccessDeniedError, XlogGapError, and a SwimError.
Now assume they have some attributes encoded with these keys:

MP_ERROR_ATTR_ACCESS_DENIED_OBJ_TYPE = 0,
MP_ERROR_ATTR_ACCESS_DENIED_OBJ_NAME = 1,
MP_ERROR_ATTR_ACCESS_DENIED_ACC_TYPE = 2,
MP_ERROR_ATTR_XLOG_GAP_VCLOCK_START = 3,
MP_ERROR_ATTR_XLOG_GAP_VCLOCK_END = 4,
MP_ERROR_ATTR_SWIM_NODE_DEAD_UUID = 5,

And now we want to add a new attribute to the AccessDeniedError
object. We will need to make it having key 6. That separates "access
denied" attributes from each other. After a few more mutations
this will be a mess.

My proposal is this:

MP_ERROR: {
    MP_ERROR_TYPE: ...,
    MP_ERROR_CUSTOM_TYPE: ...,
    MP_ERROR_TRACE: ...,
    MP_ERROR_MESSAGE: ...,
    MP_ERRNO: ...,
    MP_ERROR_CODE: ...,
    MP_ERROR_FIELDS: {
        <MP_STR>: ...,
        <MP_STR>: ...,
        ...
    }
}

As you can see, I added MP_ERROR_FIELDS, and all type-specific
fields are present here as string key + some arbitrary values.
Easy to add new type-specific attributes.

But this is arguable. I rely on my recent experience with working
with Google Cloud where they return all information as 'string key' +
'string value' except HTTP codes, and it appeared to be easy to
handle. I advice you get a second opinion from Alexander T. and
probably from Kostja Nazarov. Maybe from Mons, he knows how to do
right lots of things.

Keep in mind this is not perf critical part, so we need to be on
the best compatibility side here.

See 5 comments below, and a commit on top of the branch. Looks like
I broke something in my commit so that the new test does not pass.
Could you please find a reason?

> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index 63e8b82..de09389 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -386,6 +391,54 @@ static const struct luaL_Reg boxlib_backup[] = {
>  	{NULL, NULL}
>  };
>  
> +/**
> + * A MsgPack extensions handler that supports tuples and errors encode.
> + */
> +static enum mp_type
> +luamp_encode_extension_box(struct lua_State *L, int idx,
> +			   struct mpstream *stream)

1. Could you please move this function to a separate commit?
I see you do some tuple-related things here. Would be better to
make it beforehand to simplify this commit.

> +{
> +	struct tuple *tuple = luaT_istuple(L, idx);
> +	if (tuple != NULL) {
> +		tuple_to_mpstream(tuple, stream);
> +		return MP_ARRAY;
> +	}
> +
> +	if (current_session()->meta.serializer_opts.error_marshaling_enabled) {
> +		struct error *err = luaL_iserror(L, idx);
> +		if (err != NULL)
> +			error_to_mpstream(err, stream);
> +	}
> +
> +	return MP_EXT;
> +}
> diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
> new file mode 100644
> index 0000000..2e15206
> --- /dev/null
> +++ b/src/box/lua/mp_error.cc
> @@ -0,0 +1,373 @@
> +static struct error *
> +build_error(struct mp_error *mp_error)
> +{
> +	/*
> +	 * For create an error the "raw" constructor using
> +	 * because OOM error must be throwed in OOM case.
> +	 * Bulders returns a pointer to the static OOM error
> +	 * in OOM case.

2. Why do you want an exception out? That function is called from
error_unpack(), which is called from C. You can't throw here. The
function should be wrapped into a try-catch block. Am I missing
something?

In case there was OOM, should we return the OOM, or return NULL?

> +	 */
> +	struct error *err = NULL;
> +
> +	if (strcmp(mp_error->error_type, "ClientError") == 0) {
> +		ClientError *e = new ClientError(mp_error->file, mp_error->line,
> +						 ER_UNKNOWN);
> +		e->m_errcode = mp_error->error_code;
> +		err = e;
> +	} else if (strcmp(mp_error->error_type, "CustomError") == 0) {
> +		err = new CustomError(mp_error->file, mp_error->line,
> +				      mp_error->custom_type);
> +	} else if (strcmp(mp_error->error_type, "AccessDeniedError") == 0) {
> +		err = new AccessDeniedError(mp_error->file, mp_error->line,
> +					    mp_error->ad_access_type,
> +					    mp_error->ad_obj_type,
> +					    mp_error->ad_obj_name, "");
> +	} else if (strcmp(mp_error->error_type, "XlogError") == 0) {
> +		err = new XlogError(&type_XlogError, mp_error->file,
> +				    mp_error->line);
> +		error_format_msg(err, "%s", mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "XlogGapError") == 0) {
> +		err = new XlogGapError(mp_error->file, mp_error->line,
> +				       mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "SystemError") == 0) {
> +		err = new SystemError(mp_error->file, mp_error->line,
> +				      "%s", mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "SocketError") == 0) {
> +		err = new SocketError(mp_error->file, mp_error->line, "", "");
> +		error_format_msg(err, "%s", mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "OutOfMemory") == 0) {
> +		err = new OutOfMemory(mp_error->file, mp_error->line,
> +				      0, "", "");
> +		error_format_msg(err, "%s", mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "TimedOut") == 0) {
> +		err = new TimedOut(mp_error->file, mp_error->line);
> +	} else if (strcmp(mp_error->error_type, "ChannelIsClosed") == 0) {
> +		err = new ChannelIsClosed(mp_error->file, mp_error->line);
> +	} else if (strcmp(mp_error->error_type, "FiberIsCancelled") == 0) {
> +		err = new FiberIsCancelled(mp_error->file, mp_error->line);
> +	} else if (strcmp(mp_error->error_type, "LuajitError") == 0) {
> +		err = new LuajitError(mp_error->file, mp_error->line,
> +				      mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "IllegalParams") == 0) {
> +		err = new IllegalParams(mp_error->file, mp_error->line,
> +					"%s", mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "CollationError") == 0) {
> +		err = new CollationError(mp_error->file, mp_error->line,
> +					 "%s", mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "SwimError") == 0) {
> +		err = new SwimError(mp_error->file, mp_error->line,
> +				    "%s", mp_error->reason);
> +	} else if (strcmp(mp_error->error_type, "CryptoError") == 0) {
> +		err = new CryptoError(mp_error->file, mp_error->line,
> +				      "%s", mp_error->reason);
> +	}
> +
> +	if (err) {
> +		err->saved_errno = mp_error->saved_errno;
> +		error_format_msg(err, "%s", mp_error->reason);
> +	}
> +
> +	return err;
> +}
> +
> +struct error *
> +error_unpack(const char **data, uint32_t len)
> +{
> +	const char *end = *data + len;
> +	if (mp_typeof(**data) != MP_MAP) {
> +		diag_set(ClientError, ER_INVALID_MSGPACK,
> +			 "Invalid MP_ERROR format");
> +		return NULL;
> +	}
> +
> +	struct mp_error mp_err;
> +	mp_error_init(&mp_err);
> +
> +	uint32_t map_size = mp_decode_map(data);
> +
> +	struct error *err = NULL;
> +	for (uint32_t i = 0; i < map_size; ++i) {
> +		if (mp_typeof(**data) != MP_UINT) {
> +			diag_set(ClientError, ER_INVALID_MSGPACK,
> +				 "Invalid MP_ERROR MsgPack format");
> +			return NULL;
> +		}
> +
> +		uint8_t key = mp_decode_uint(data);
> +		const char *str;
> +		uint32_t str_len;
> +		switch(key) {
> +		case MP_ERROR_DET_TYPE:
> +			if (mp_typeof(**data) != MP_STR)
> +				goto error;

3. That is actually an open question whether we should validate
MessagePack here. Currently none of MP_EXT decoders do that in
luamp_decode. Normal types also skip validation.

I am not saying it should not be fixed, but probably everywhere
at once, in a centralized way.

> +			str = mp_decode_str(data, &str_len);
> +			mp_err.error_type = strndup(str, str_len);
> +			break;
> +		case MP_ERROR_DET_FILE:
> +			if (mp_typeof(**data) != MP_STR)
> +				goto error;
> +			str = mp_decode_str(data, &str_len);
> +			mp_err.file = strndup(str, str_len);
> +			break;
> diff --git a/src/lua/error.h b/src/lua/error.h
> index 16cdaf7..4e4dc04 100644
> --- a/src/lua/error.h
> +++ b/src/lua/error.h
> @@ -37,7 +37,6 @@
>  extern "C" {
>  #endif /* defined(__cplusplus) */
>  
> -

4. Please, omit unnecessary diff.

>  /** \cond public */
>  struct error;
>  
> diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
> new file mode 100755
> index 0000000..eb86bdc
> --- /dev/null
> +++ b/test/box-tap/extended_error.test.lua

5. Not sure how does it work now, taking into account that you
didn't update iproto part.

Please, add tests which use msgpack and msgpackffi. Without
network. Marshaling should affect it too.

Also add tests when happens when marshaling is disabled, and
yet a session tries to decode MP_ERROR. That may happen, if
it obtains the MessagePack generated in another session, or
if you turn marshaling on and off for one session.

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

diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index de09389ef..5e04987fe 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -423,7 +423,7 @@ luamp_decode_extension_box(struct lua_State *L, const char **data)
 	int8_t ext_type;
 	uint32_t len = mp_decode_extl(data, &ext_type);
 
-	if(ext_type != MP_ERROR) {
+	if (ext_type != MP_ERROR) {
 		luaL_error(L, "Unsuported MsgPack extension type: %u",
 			   ext_type);
 		return;
diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
index 2e1520621..38a6f6e98 100644
--- a/src/box/lua/mp_error.cc
+++ b/src/box/lua/mp_error.cc
@@ -33,21 +33,22 @@
 #include "mpstream/mpstream.h"
 #include "msgpuck.h"
 #include "mp_extension_types.h"
+#include "fiber.h"
 
 /**
  * Keys used for encode/decode MP_ERROR.
  */
-enum mp_error_details {
-	MP_ERROR_DET_TYPE,
-	MP_ERROR_DET_FILE,
-	MP_ERROR_DET_LINE,
-	MP_ERROR_DET_REASON,
-	MP_ERROR_DET_ERRNO,
-	MP_ERROR_DET_CODE,
-	MP_ERROR_DET_CUSTOM_TYPE,
-	MP_ERROR_DET_AD_OBJ_TYPE,
-	MP_ERROR_DET_AD_OBJ_NAME,
-	MP_ERROR_DET_AD_ACCESS_TYPE
+enum {
+	MP_ERROR_TYPE,
+	MP_ERROR_FILE,
+	MP_ERROR_LINE,
+	MP_ERROR_REASON,
+	MP_ERROR_ERRNO,
+	MP_ERROR_CODE,
+	MP_ERROR_CUSTOM_TYPE,
+	MP_ERROR_AD_OBJ_TYPE,
+	MP_ERROR_AD_OBJ_NAME,
+	MP_ERROR_AD_ACCESS_TYPE
 };
 
 /**
@@ -58,40 +59,19 @@ struct mp_error {
 	uint32_t error_code;
 	uint32_t line;
 	uint32_t saved_errno;
-	char *error_type;
-	char *file;
-	char *reason;
-	char *custom_type;
-	char *ad_obj_type;
-	char *ad_obj_name;
-	char *ad_access_type;
+	const char *error_type;
+	const char *file;
+	const char *reason;
+	const char *custom_type;
+	const char *ad_obj_type;
+	const char *ad_obj_name;
+	const char *ad_access_type;
 };
 
 static void
-mp_error_init(struct mp_error *mp_error)
+mp_error_create(struct mp_error *mp_error)
 {
-	mp_error->error_code = 0;
-	mp_error->line = 0;
-	mp_error->saved_errno = 0;
-	mp_error->error_type = NULL;
-	mp_error->file = NULL;
-	mp_error->reason = NULL;
-	mp_error->custom_type = NULL;
-	mp_error->ad_obj_type = NULL;
-	mp_error->ad_obj_name = NULL;
-	mp_error->ad_access_type = NULL;
-}
-
-static void
-mp_error_cleanup(struct mp_error *mp_error)
-{
-	free(mp_error->error_type);
-	free(mp_error->file);
-	free(mp_error->reason);
-	free(mp_error->custom_type);
-	free(mp_error->ad_obj_type);
-	free(mp_error->ad_obj_name);
-	free(mp_error->ad_access_type);
+	memset(mp_error, 0, sizeof(*mp_error));
 }
 
 void
@@ -118,25 +98,25 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
 	uint32_t details_num = 5;
 	uint32_t data_size = 0;
 
-	data_size += mp_sizeof_uint(MP_ERROR_DET_TYPE);
+	data_size += mp_sizeof_uint(MP_ERROR_TYPE);
 	data_size += mp_sizeof_str(strlen(error->type->name));
-	data_size += mp_sizeof_uint(MP_ERROR_DET_LINE);
+	data_size += mp_sizeof_uint(MP_ERROR_LINE);
 	data_size += mp_sizeof_uint(error->line);
-	data_size += mp_sizeof_uint(MP_ERROR_DET_FILE);
+	data_size += mp_sizeof_uint(MP_ERROR_FILE);
 	data_size += mp_sizeof_str(strlen(error->file));
-	data_size += mp_sizeof_uint(MP_ERROR_DET_REASON);
+	data_size += mp_sizeof_uint(MP_ERROR_REASON);
 	data_size += mp_sizeof_str(strlen(error->errmsg));
-	data_size += mp_sizeof_uint(MP_ERROR_DET_ERRNO);
+	data_size += mp_sizeof_uint(MP_ERROR_ERRNO);
 	data_size += mp_sizeof_uint(error->saved_errno);
 
 	if (is_client || is_access_denied || is_custom) {
 		++details_num;
 		errcode = box_error_code(error);
-		data_size += mp_sizeof_uint(MP_ERROR_DET_CODE);
+		data_size += mp_sizeof_uint(MP_ERROR_CODE);
 		data_size += mp_sizeof_uint(errcode);
 		if (is_custom) {
 			++details_num;
-			data_size += mp_sizeof_uint(MP_ERROR_DET_CUSTOM_TYPE);
+			data_size += mp_sizeof_uint(MP_ERROR_CUSTOM_TYPE);
 			custom_type = box_error_custom_type(error);
 			data_size += mp_sizeof_str(strlen(custom_type));
 		} else if (is_access_denied) {
@@ -146,11 +126,11 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
 			ad_obj_type = ad_err->object_type();
 			ad_obj_name = ad_err->object_name();
 			ad_access_type = ad_err->access_type();
-			data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_TYPE);
+			data_size += mp_sizeof_uint(MP_ERROR_AD_OBJ_TYPE);
 			data_size += mp_sizeof_str(strlen(ad_obj_type));
-			data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_NAME);
+			data_size += mp_sizeof_uint(MP_ERROR_AD_OBJ_NAME);
 			data_size += mp_sizeof_str(strlen(ad_obj_name));
-			data_size += mp_sizeof_uint(MP_ERROR_DET_AD_ACCESS_TYPE);
+			data_size += mp_sizeof_uint(MP_ERROR_AD_ACCESS_TYPE);
 			data_size += mp_sizeof_str(strlen(ad_access_type));
 		}
 	}
@@ -162,33 +142,33 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
 	char *data = ptr;
 	data = mp_encode_extl(data, MP_ERROR, data_size);
 	data = mp_encode_map(data, details_num);
-	data = mp_encode_uint(data, MP_ERROR_DET_TYPE);
+	data = mp_encode_uint(data, MP_ERROR_TYPE);
 	data = mp_encode_str(data, error->type->name,
 			     strlen(error->type->name));
-	data = mp_encode_uint(data, MP_ERROR_DET_LINE);
+	data = mp_encode_uint(data, MP_ERROR_LINE);
 	data = mp_encode_uint(data, error->line);
-	data = mp_encode_uint(data, MP_ERROR_DET_FILE);
+	data = mp_encode_uint(data, MP_ERROR_FILE);
 	data = mp_encode_str(data, error->file, strlen(error->file));
-	data = mp_encode_uint(data, MP_ERROR_DET_REASON);
+	data = mp_encode_uint(data, MP_ERROR_REASON);
 	data = mp_encode_str(data, error->errmsg, strlen(error->errmsg));
-	data = mp_encode_uint(data, MP_ERROR_DET_ERRNO);
+	data = mp_encode_uint(data, MP_ERROR_ERRNO);
 	data = mp_encode_uint(data, error->saved_errno);
 
 	if (is_client || is_access_denied || is_custom) {
-		data = mp_encode_uint(data, MP_ERROR_DET_CODE);
+		data = mp_encode_uint(data, MP_ERROR_CODE);
 		data = mp_encode_uint(data, errcode);
 		if (is_custom) {
-			data = mp_encode_uint(data, MP_ERROR_DET_CUSTOM_TYPE);
+			data = mp_encode_uint(data, MP_ERROR_CUSTOM_TYPE);
 			data = mp_encode_str(data, custom_type,
 					     strlen(custom_type));
 		} else if (is_access_denied) {
-			data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_TYPE);
+			data = mp_encode_uint(data, MP_ERROR_AD_OBJ_TYPE);
 			data = mp_encode_str(data, ad_obj_type,
 					     strlen(ad_obj_type));
-			data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_NAME);
+			data = mp_encode_uint(data, MP_ERROR_AD_OBJ_NAME);
 			data = mp_encode_str(data, ad_obj_name,
 					     strlen(ad_obj_name));
-			data = mp_encode_uint(data, MP_ERROR_DET_AD_ACCESS_TYPE);
+			data = mp_encode_uint(data, MP_ERROR_AD_ACCESS_TYPE);
 			data = mp_encode_str(data, ad_access_type,
 					     strlen(ad_access_type));
 		}
@@ -202,9 +182,9 @@ static struct error *
 build_error(struct mp_error *mp_error)
 {
 	/*
-	 * For create an error the "raw" constructor using
-	 * because OOM error must be throwed in OOM case.
-	 * Bulders returns a pointer to the static OOM error
+	 * To create an error the "raw" constructor using
+	 * because OOM error must be thrown in OOM case.
+	 * Builders returns a pointer to the static OOM error
 	 * in OOM case.
 	 */
 	struct error *err = NULL;
@@ -270,8 +250,21 @@ build_error(struct mp_error *mp_error)
 	return err;
 }
 
+static inline const char *
+region_strdup(struct region *region, const char *str, uint32_t len)
+{
+	char *res = (char *)region_alloc(region, len + 1);
+	if (res == NULL) {
+		diag_set(OutOfMemory, len + 1, "region_alloc", "res");
+		return NULL;
+	}
+	memcpy(res, str, len);
+	res[len] = 0;
+	return res;
+}
+
 struct error *
-error_unpack(const char **data, uint32_t len)
+error_unpack_xc(const char **data, uint32_t len)
 {
 	const char *end = *data + len;
 	if (mp_typeof(**data) != MP_MAP) {
@@ -281,8 +274,9 @@ error_unpack(const char **data, uint32_t len)
 	}
 
 	struct mp_error mp_err;
-	mp_error_init(&mp_err);
-
+	mp_error_create(&mp_err);
+	struct region *region = &fiber()->gc;
+	uint32_t region_svp = region_used(region);
 	uint32_t map_size = mp_decode_map(data);
 
 	struct error *err = NULL;
@@ -290,69 +284,87 @@ error_unpack(const char **data, uint32_t len)
 		if (mp_typeof(**data) != MP_UINT) {
 			diag_set(ClientError, ER_INVALID_MSGPACK,
 				 "Invalid MP_ERROR MsgPack format");
-			return NULL;
+			goto finish;
 		}
 
-		uint8_t key = mp_decode_uint(data);
+		uint64_t key = mp_decode_uint(data);
 		const char *str;
 		uint32_t str_len;
 		switch(key) {
-		case MP_ERROR_DET_TYPE:
+		case MP_ERROR_TYPE:
 			if (mp_typeof(**data) != MP_STR)
 				goto error;
 			str = mp_decode_str(data, &str_len);
-			mp_err.error_type = strndup(str, str_len);
+			mp_err.error_type = region_strdup(region, str, str_len);
+			if (mp_err.error_type == NULL)
+				goto finish;
 			break;
-		case MP_ERROR_DET_FILE:
+		case MP_ERROR_FILE:
 			if (mp_typeof(**data) != MP_STR)
 				goto error;
 			str = mp_decode_str(data, &str_len);
-			mp_err.file = strndup(str, str_len);
+			mp_err.file = region_strdup(region, str, str_len);
+			if (mp_err.file == NULL)
+				goto finish;
 			break;
-		case MP_ERROR_DET_LINE:
+		case MP_ERROR_LINE:
 			if (mp_typeof(**data) != MP_UINT)
 				goto error;
 			mp_err.line = mp_decode_uint(data);
 			break;
-		case MP_ERROR_DET_REASON:
+		case MP_ERROR_REASON:
 			if (mp_typeof(**data) != MP_STR)
 				goto error;
 			str = mp_decode_str(data, &str_len);
-			mp_err.reason = strndup(str, str_len);
+			mp_err.reason = region_strdup(region, str, str_len);
+			if (mp_err.reason == NULL)
+				goto finish;
 			break;
-		case MP_ERROR_DET_ERRNO:
+		case MP_ERROR_ERRNO:
 			if (mp_typeof(**data) != MP_UINT)
 				goto error;
 			mp_err.saved_errno = mp_decode_uint(data);
 			break;
-		case MP_ERROR_DET_CODE:
+		case MP_ERROR_CODE:
 			if (mp_typeof(**data) != MP_UINT)
 				goto error;
 			mp_err.error_code = mp_decode_uint(data);
 			break;
-		case MP_ERROR_DET_CUSTOM_TYPE:
+		case MP_ERROR_CUSTOM_TYPE:
 			if (mp_typeof(**data) != MP_STR)
 				goto error;
 			str = mp_decode_str(data, &str_len);
-			mp_err.custom_type = strndup(str, str_len);
+			mp_err.custom_type = region_strdup(region, str,
+							   str_len);
+			if (mp_err.custom_type == NULL)
+				goto finish;
 			break;
-		case MP_ERROR_DET_AD_OBJ_TYPE:
+		case MP_ERROR_AD_OBJ_TYPE:
 			if (mp_typeof(**data) != MP_STR)
 				goto error;
 			str = mp_decode_str(data, &str_len);
-			mp_err.ad_obj_type = strndup(str, str_len);
+			mp_err.ad_obj_type = region_strdup(region, str,
+							   str_len);
+			if (mp_err.ad_obj_type == NULL)
+				goto finish;
 			break;
-		case MP_ERROR_DET_AD_OBJ_NAME:
+		case MP_ERROR_AD_OBJ_NAME:
 			if (mp_typeof(**data) != MP_STR)
 				goto error;
 			str = mp_decode_str(data, &str_len);
-			mp_err.ad_obj_name = strndup(str, str_len);
+			mp_err.ad_obj_name = region_strdup(region, str,
+							   str_len);
+			if (mp_err.ad_obj_name == NULL)
+				goto finish;
 			break;
-		case MP_ERROR_DET_AD_ACCESS_TYPE:
+		case MP_ERROR_AD_ACCESS_TYPE:
 			if (mp_typeof(**data) != MP_STR)
 				goto error;
 			str = mp_decode_str(data, &str_len);
-			mp_err.ad_access_type = strndup(str, str_len);
+			mp_err.ad_access_type = region_strdup(region, str,
+							      str_len);
+			if (mp_err.ad_access_type == NULL)
+				goto finish;
 			break;
 		default:
 			mp_next(data);
@@ -363,11 +375,22 @@ error_unpack(const char **data, uint32_t len)
 	assert(*data == end);
 
 	err = build_error(&mp_err);
-	mp_error_cleanup(&mp_err);
+finish:
+	region_truncate(region, region_svp);
 	return err;
 
 error:
 	diag_set(ClientError, ER_INVALID_MSGPACK,
 		 "Invalid MP_ERROR MsgPack format");
-	return NULL;
+	goto finish;
+}
+
+struct error *
+error_unpack(const char **data, uint32_t len)
+{
+	try {
+		return error_unpack_xc(data, len);
+	} catch (OutOfMemory *e) {
+		return e;
+	}
 }
diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
index eb86bdc76..8a4f79c04 100755
--- a/test/box-tap/extended_error.test.lua
+++ b/test/box-tap/extended_error.test.lua
@@ -3,12 +3,10 @@
 local netbox = require('net.box')
 local os = require('os')
 local tap = require('tap')
-local uri = require('uri')
 
 local test = tap.test('check an extended error')
 test:plan(4)
 
-
 function error_func()
     box.error(box.error.PROC_LUA, "An old good error")
 end
@@ -22,22 +20,6 @@ function custom_error_func_2()
     return err
 end
 
-local function version_at_least(peer_version_str, major, minor, patch)
-    local major_p, minor_p, patch_p = string.match(peer_version_str,
-                                                   "(%d)%.(%d)%.(%d)")
-    major_p = tonumber(major_p)
-    minor_p = tonumber(minor_p)
-    patch_p = tonumber(patch_p)
-
-    if major_p < major
-        or (major_p == major and minor_p < minor)
-        or (major_p == major and minor_p == minor and patch_p < patch) then
-            return false
-    end
-
-    return true
-end
-
 local function grant_func(user, name)
     box.schema.func.create(name, {if_not_exists = true})
     box.schema.user.grant(user, 'execute', 'function', name, {
@@ -85,15 +67,14 @@ local function check_error(err, check_list)
     return true
 end
 
-local function test_old_transmission(host, port)
+local function test_old_transmission()
     grant_func('guest', 'error_func')
     grant_func('guest', 'custom_error_func_2')
 
-    local connection = netbox.connect(host, port)
+    local connection = netbox.connect(box.cfg.listen)
     local _, err = pcall(connection.call, connection, 'error_func')
     local err_2 = connection:call('custom_error_func_2')
 
-
     local check_list = {
         type = 'ClientError',
         message = 'An old good error',
@@ -111,12 +92,12 @@ local function test_old_transmission(host, port)
     connection:close()
 end
 
-local function test_extended_transmission(host, port)
+local function test_extended_transmission()
     grant_func('guest', 'custom_error_func')
     grant_func('guest', 'custom_error_func_2')
     box.schema.user.grant('guest','read,write', 'space', '_session_settings')
 
-    local connection = netbox.connect(host, port)
+    local connection = netbox.connect(box.cfg.listen)
     connection.space._session_settings:update('error_marshaling_enabled',
                                               {{'=', 2, true}})
     local _, err = pcall(connection.call, connection, 'custom_error_func')
@@ -148,10 +129,8 @@ end
 box.cfg{
     listen = os.getenv('LISTEN')
 }
-local host= uri.parse(box.cfg.listen).host or 'localhost'
-local port = uri.parse(box.cfg.listen).service 
 
-test_extended_transmission(host, port)
-test_old_transmission(host, port)
+test_extended_transmission()
+test_old_transmission()
 
 os.exit(test:check() and 0 or 1)