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

[lvasiliev <lvasiliev@tarantool.org>]

Hi! Thanks for the review.

On 17.04.2020 3:58, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> One major note: mpstream_iproto_encode_error() is not
> updated at all. So the marshaling is still not complete.
> 
I tried. If we add mp_ext to IPROTO_ERROR, we’ll break old clients.
> 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.
> 
Added
>> Additional fields:
>> - ClientError code
>> - Traceback (now only Lua traceback)
> 
> Traceback is not here anymore.
> 
Discussed
>> - 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>: ...,
>          ...
>      }
> }
> 
Implemented
> 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.
> 
With a separate commit, we move one function from file A to file B for
  modify it in the next commit. Sounds weird.
>> +{
>> +	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?
Null and set OOM to diag.
> 
>> +	 */
>> +	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.
Tests inprogress.
> 
> ====================
> 
> 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)
>