Redis branch: latest unstable at commit c552fad

Issue statement

It appears that redis module API RedisModule_HashSet() was returning 0 for the creation of a new hash type key which did not previously exist. It however returns the number of fields updated for a hash type key which previsouly existed. Since a return value of 0 is currently considered an error by RedisModule_HashSet(), it becomes confusing to determine whether the HashSet is successful or not based on the return value.

Expected behavior

The return value for a successful new hash key creation by RedisModule_HashSet() shoud differ from the error return code.

To reproduce the issue

                int ret;
                RedisModuleKey *hash_key_ptr = RedisModule_OpenKey(ctx, hash_key,
                    REDISMODULE_READ|REDISMODULE_WRITE);
                keytype = RedisModule_KeyType(hash_key_ptr);

                if (keytype != REDISMODULE_KEYTYPE_HASH &&
                    keytype != REDISMODULE_KEYTYPE_EMPTY)
                {
                    RedisModule_CloseKey(hash_key_ptr);
                    return RedisModule_ReplyWithError(ctx,REDISMODULE_ERRORMSG_WRONGTYPE);
                }

                ret = RedisModule_HashSet(hash_key_ptr, REDISMODULE_HASH_CFIELDS,
                                          "field1", value1,
                                          "field2", value2,
                                          "field3", value3,
                                          NULL);

Return value ret was found to be 0. The hash key was found to be written successfully.

Root cause analysis

module.c

int RM_HashSet(RedisModuleKey key, int flags, ...) { ... 2783 int updated = 0; ... 2823 robj argv[2] = {field,value}; 2824 hashTypeTryConversion(key->value,argv,0,1); 2825 updated += hashTypeSet(key->value, field->ptr, value->ptr, low_flags); ... 2836 return updated;

hashTypeSet() returns the number of updated fields ONLY (i.e. the field must previsously exist). If the field is new (in the case of creating a new hash key), hashTypeSet returns 0 even though the field is created. Hence for RM_HashSet(), variable "updated" was not incremented for any field when creating a new hash key and 0 is returned to caller, although the hash key creation was successful.

Comment From: trevor211

Asking the core team to take a look at here. @oranagra @yossigo @itamarhaber @soloestoy @madolson

Comment From: oranagra

indeed the API return value is ambitious. the only way i can see to fix it without breaking backward compatibility or adding a new API is maybe set errno when returning 0? @guybe7 any thoughts?

Comment From: guybe7

i vote for setting errno in case of an error (e.g. key is not a hash) the usage pattern should be:

if (RedisModule_HashSet(..) == 0 && errno != 0) {
    // error
}

Comment From: yossigo

I don't like the abuse of errno for these purposes, it's a case of code smell at best and could also introduce real issues (and yes I realize it's already practiced in some cases).

Consider this a bug and fix it. If existing modules rely on a zero return value when the hash is created.
Add a new flag that modifies this behavior. This would be backwards compatible but very awkward.

Comment From: oranagra

@yossigo i'm not sure what you are suggesting. The only ways i see around it are: 1. Add a new API that has two return values, and deprecate the old one. 2. Break the API so that the return value is just an indication of success / failure, and there's no indication of how many fields were updated.

Comment From: yossigo

The documentation specifies RM_HashSet() should return the number of fields updated, but fails to specify that inserted keys are not counted as updated.

This gets further complicated when we consider HSET itself returns the number of keys inserted rather than the number of keys updated.

We can have a REDISMODULE_HASH_COUNT_INSERTS additional flag that counts insertions in addition to updates when returning the number of fields. This will solve the problem in a backwards compatible but kind of ugly way.

Comment From: guybe7

maybe @yossigo's suggestion is the lesser of two evils...

Comment From: oranagra

But if it returns either insertions or updates, what about an error? I suppose that if the caller asked for the number of insertions and we return 0, that's the same as error, but it sounds off that we still provide the other option (get the count of updates) in which there's no way to distinguish between an error and success. If we're breaking the API anyway by adding a new flag, we can add another return value. Or just say that the new API returns just the insertions (similar to HSET) and not support the other count at all.

Maybe considering that the documentation wasn't clear on what "updates" are, we can just call it a bug and "fix" it to include insertions, mention it in some release notes and be done with it? I'm imagining not many will be affected by this change.

Comment From: yossigo

Looking at all modules listed in redis.io, none of them would indeed break by this change but I'm not sure this makes me feel more comfortable with introducing such breaking changes.

The main problem I see with the flag that newer modules will end up incompatible with older Redis versions, which will silently ignore the flag.

I also wish to avoid adding more functions as a way to solve bugs, so the only solution we're left with is to start planning REDISMODULE_APIVER_2 and assign breaking changes to it, so we can push it forward when the amount of pending changes justifies the effort.

Comment From: zuiderkwast

Setting errno sounds like a good idea, since we decided to go that way for the new streams API functions. Adding a flag also sounds like a good idea. I think we can do both.

The main problem I see with the flag that newer modules will end up incompatible with older Redis versions, which will silently ignore the flag.

Newer modules are always incompatible with older Redis versions. For example, they may use functions which didn't exist in the old version. Those errors are not silently ignored though, so I guess it's not as bad.

Actually, even setting errno isn't fully backward compatible, because then you can check for success by setting errno = 0 before the call, then check it afterwards, but this trick doesn't work in old Redis versions which doesn't set errno.

I think we can live with this very limited backward compatibility issue since the whole module API is sort of beta still, or start incrementing the module version on every change. There's no reason to wait until version 1 is final IMHO. Another option is to simply document in which Redis version each flag (etc.) was added in cases like this.

Comment From: oranagra

@zuiderkwast FYI modules compiled with new module api can still work with old version of Redis. They can simply check is a certain api exists by checking if it's NULL. Also see #7865

I guess the conclusion of this discussion is to add a new flag, and also use errno, and maybe document that these changes exist since a certain version of Redis. Going forward (starting with this version) we should treat unsupported flags as an error.

Redis Redis module RM_HashSet() return confusing 0 for hash creation

Issue statement

Expected behavior

To reproduce the issue

Root cause analysis