search

AliBazzi / IdentityServer4.Contrib.RedisStore

A persistence layer using Redis DB for operational data and for caching capability for Identity Server 4
https://www.nuget.org/packages/IdentityServer4.Contrib.RedisStore
MIT License
137 stars 48 forks source link

Possible mis-use of async await #10

Closed dpix closed 6 years ago

dpix commented 6 years ago

I've been using this library in production for the past few weeks, and has been great, easy to set up and currently handling ~40k tokens in storage.

I have started seeing some issues with our identity provider, not sure if it is related to this library at all but just pulled the code to check it out.

Seems that you are missing a lot of await calls on async redis database transaction methods: e.g. https://github.com/AliBazzi/IdentityServer4.Contrib.RedisStore/blob/master/IdentityServer4.Contrib.RedisStore/Stores/PersistedGrantStore.cs#L64

Is this on purpose? I suspect it could well cause the transaction to get executed with only some of the actions applied. In the example above StringSetAsync, SetAddAsync etc are all missing await operators.

AliBazzi commented 6 years ago

Hi @dpix

For your concern, it's on purpose, because StackExchange.Redis prepares all the operations before the library calls Redis to execute a bunch of operations as one transaction by calling await transaction.ExecuteAsync(), so no need to await StringSetAsync, SetAddAsync etc, and this is applied according to StackExchange.Redis docs.

Please let me know what other concerns you have, happy to help.

AliBazzi commented 6 years ago

Hi @dpix I will close this issue, please feel free to open it again to raise more specific concerns if you have any.