[Col-Waltz]

call.map() passed request_timeout to vshard, but didn't pass timeout.
Vshard then fell back to CALL_TIMEOUT_MAX (64s) and could crash on
assert(request_timeout <= timeout) when request_timeout > 64 inside
vshard call.

Pass timeout to vshard call options in call.map() to make map calls
consistent with call.single() and to keep CRUD and vshard time budgets
aligned (CRUD default is 2s, vshard default is 64s).

Also validate request_timeout <= timeout early and return a clear error
instead of failed assertion.

[Serpentian]

Thank you for the patch!

[Serpentian]

There's call.single, which still passes request_timeout > timeout to vshard unchanged. So, the assertion failure can be still reproduced with crud.get() and crud.locate()

[Col-Waltz]

Thanks, added check also for call.single

[Serpentian]

crud/crud/select/merger.lua

Lines 204 to 208 in e7b4b8b

	local net_box_opts = {is_async = true, buffer = buf,
	skip_header = utils.tarantool_supports_netbox_skip_header_option() or nil}
	local func_args_ext = utils.append_array({ box.session.effective_user(), func_name }, func_args)
	local future = replicaset[vshard_call_name](replicaset, "_crud.call_on_storage",
	func_args_ext, net_box_opts)

The merger-backed select and pairs path calls replicaset methods directly. Its net_box_opts contains neither timeout nor request_timeout, although their public options are accepted and carried into call_opts. Consequently, this path bypasses the new map validation and leaves vshard using its own timeout defaults.

[Serpentian]

And btw in merge the default timeout is also nil:

crud/crud/select/merger.lua

Line 112 in e7b4b8b

local timeout = context.timeout or call.DEFAULT_VSHARD_CALL_TIMEOUT

There's a typo, should be const instead of call

[Col-Waltz]

Thanks, fixed typo and also added timeout to new merger and new_readview because they did not pass timeout to the vshard. Also added checks for select functions which are susceptible to the same issue with vshard assertion return. Checks are performed once a function call before any heavy logic is performed and souldn't affect perf much.

[Serpentian]

Unfortunately, it's still the hot-path. The same way it's in vshard. And now we add if, which always be executed.

See, how crud uses the dev_checks for checking the options, And these checks are disabled on real production. However, there's still public entrypoints (e.g. crud.update.call), and they use the ordinary checks module, they're always launched.

So, the perfect solution imho should be some extension of the checks in crud, which will allow specifying the function, which will receive the whole opts table and compare request_timeout and timeout. And these checks under dev_checks will also be enabled only if TARANTOOL_CRUD_ENABLE_INTERNAL_CHECKS=ON.

But I'd rather ask smb from crud team, whether we should care that much. Maybe @vakhov

P.S. These checks are not in vshard since #521 (comment)

[vakhov]

I'd keep the current check without extending checks. Before making the final decision, though, we should build the patched version and benchmark it against the reference to measure any performance regression. This is a separate task, and I've already contacted Ilya about it.

[Serpentian]

I'd keep the current check without extending checks.

Which means that this will be LuaJIT assertion failure, when request_timeout > timeout. Are we ok with such behavior?

[vakhov]

By "the current check" I meant the explicit validation added in this PR. We should add the same validation to call.single() so that invalid timeout combinations produce a clear error there too. My suggestion was to do this with a plain if, without extending checks.

[Col-Waltz]

I agree that this checks can affect performance. I refactored checks having the least severe impact on performance. I performed some perf tests and there is no rate reduction more than 1% even on cold run. But after we make a proper perf testing we can remove this checks or add a flag to crud/common/dev_checks and hide this checks under this flag to not use them on production.