Execute empty MULTI

[Mik13]

Description

Currently, calling .exec() on an "empty" multi command (without calling any other command on it), is not executing MULTI EXEC on the server.
This can cause problems when using WATCH, because no MULTI EXEC means the key is still watched.

Fix

Execute MULTI EXEC even if the multi is empty.

[leibale]

@Mik13 good catch!

1. IMO executing MULTI EXEC (instead of UNWATCH) is cleaner.
2. I don't like executing commands when not needed (if there is no WATCH, there is no reason to UNWATCH/MULTI EXEC).

I need to think about that more...

[Mik13]

@Mik13 good catch!

1. IMO executing MULTI EXEC (instead of UNWATCH) is cleaner.
2. I don't like executing commands when not needed (if there is no WATCH, there is no reason to UNWATCH/MULTI EXEC).

I need to think about that more...

Hi @leibale!

To 1: Agree
To 2: Same, thats why I am using one O(1) command instead of O(1) + "Depends on commands in the transaction"

I do not understand the project good enough how we could possibly catch the watch inside of the multi-command when issued outside of course. That would be perfect, if it does not add too much complexity.

[leibale]

@Mik13 checking if WATCH has been called before won't be easy without losing performance in the "normal case".. but maybe the best way to avoid this is to:

// instead of:
const multi = client.multi();
await client.watch('key');
if (shouldRun()) {
  multi.set('a', 'b');
}
await multi.exec();

// do:
if (shouldRun()) {
  await Promise.all([
    client.watch('key'),
    client.multi()
      .set('a', 'b')
      .exec();
  ]);
}

@guyroyse we need your opinion here.. :)

[guyroyse]

My thought is that we should call what the user told us to call. If I call MULTI and then call EXEC without doing anything else, then that's what I asked for and that's what I should get. It's cleaner and, more importantly, provides unsurprising behavior to the user.

[leibale]

@Mik13 @guyroyse review my changes please? 🙏

[codecov-commenter]

Codecov Report

Base: 95.60% // Head: 95.59% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (35ba863) compared to base (1be8422).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 35ba863 differs from pull request most recent head 8fc6bf3. Consider uploading reports for the commit 8fc6bf3 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2423      +/-   ##
==========================================
- Coverage   95.60%   95.59%   -0.01%     
==========================================
  Files         455      455              
  Lines        4550     4545       -5     
  Branches      520      519       -1     
==========================================
- Hits         4350     4345       -5     
- Misses        128      130       +2     
+ Partials       72       70       -2     

Impacted Files	Coverage Δ
packages/client/lib/cluster/multi-command.ts	66.66% <ø> (+1.04%)	⬆️
packages/client/lib/multi-command.ts	80.00% <ø> (-1.82%)	⬇️
packages/client/lib/client/index.ts	92.33% <100.00%> (+0.02%)	⬆️
packages/client/lib/client/multi-command.ts	90.19% <100.00%> (+1.73%)	⬆️
packages/client/lib/cluster/cluster-slots.ts	79.11% <0.00%> (-0.89%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[guyroyse]

The lack of a space before as in places like [C in keyof M[P]as ExcludeMappedString<C>] makes me twitch a bit but this looks good to me otherwise.

[Mik13]

Looks good to me 👍
Thank you two for the fast responses!