🩹 Fix: race condition in memory storage

[Hrily]

Description

This commit updates Conn method of memory storage to fix race conditions.

The Conn method returned the reference to underlying map which could be used
for read/write and had a risk of race condition.

This commit fixes it by updating the method to take in a function to access
underlying map, which is wrapped in lock to make it thread safe.

Fixes: #2400

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• For new functionalities I follow the inspiration of the express js framework and built them similar in usage
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
• I tried to make my code as fast as possible with as few allocations as possible
• For new code I have written benchmarks so that they can be analyzed and improved

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

[welcome]

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[gaby]

This is an interface, we shouldn't rename it to avoid breaking changes.

[Hrily]

@gaby What interface does this implement?

I can see the earlier Conn method only on this struct
https://sourcegraph.com/search?q=context:global+repo:https://github.com/gofiber/fiber/+%22Conn()%22&patternType=standard&sm=1&groupBy=repo

[gaby]

https://github.com/gofiber/storage/blob/main/storage.go

We may need to discuss this, since it's implemented differently in multiple places 🤔

[Hrily]

As far as I understand, this method is only provided by memory.Storage, because I don't think any other storage will return a Conn of type map[string]entry.

[ReneWerner87]

no this method is provided by all adapters
https://github.com/search?q=repo%3Agofiber%2Fstorage%20conn&type=code

[Hrily]

I see

I think we can make Storage.db a sync.Map so we can return it as it is from Conn(), without worrying about thread safety.

Thoughts @gaby , @ReneWerner87 ?

[Hrily]

A ping on this in case it was missed. @gaby @ReneWerner87

[ReneWerner87]

sync.Map is ok, but slower or ?

[Hrily]

I can run the Benchmark_Storage_Memory and see if it's better or worse.

[Hrily]

@ReneWerner87 sync.Map based implementation is slower than existing implementation for Benchmark_Storage_Memory:

Existing Implementation Results:

sync.Map based implementation Results:

But our choice should not depend on the benchmark we have. From sync.Map Documentation:

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

So if Storage's use case aligns with one of the above two points, we can go ahead and use sync.Map. Else we can have another interface layer on top of vanilla map with RWMutex which we can return from Conn()

[ReneWerner87]

i just thought about the bug (#2360) again
can you please first create a test case which produces the error

with the sync.map I don't know yet, is already a considerable time loss for the driver which should normally be fastest

[ReneWerner87]

maybe it would also be ok if we modify the memory storage driver, as long as we don't modify the internal memory driver which is the default one

[Hrily]

can you please first create a test case which produces the error

@ReneWerner87 Test_Storage_Memory_ConnLocked_Race tests for this. It was failing for current Conn() implementation, i.e. with race error of reading while writing to map.

[ReneWerner87]

mutext handling should take place outside or with an adjustment of the storage interface in the storage project