andersfylling / disgord

Go module for interacting with the documented Discord's bot interface; Gateway, REST requests and voice
BSD 3-Clause "New" or "Revised" License
502 stars 71 forks source link

Race issue in LFU cache replacement strategy #385

Closed andersfylling closed 3 years ago

andersfylling commented 3 years ago

I just realized this. Take note of the LFU Delete method: image

And how mutexes are split between the set of cached objects to reduce blocking: image

The idea here is to lock the set on writes, such as deletion or creation. While changes to objects within the set would use the "shardedMutex" struct, without caring whether or not the set is blocked or not.

Taking GetChannel as an example: image

We check for locks again the set, fetch the content, and then check for "possible" locks against the mutex shard for the channel object. The issue is when a delete takes place. While the set is locked, the .Val is written to without locking the relevant shard mutex. Since this is abstracted away from each other, there is no way to directly fix this without calling a mutex lock on the set and the mutex shard at the same time.

If this race happens, there will be a casting panic as the channel might have existed on extraction, but has been defined as nil before copying.

andersfylling commented 3 years ago

Possible fix: image

Simply reference the direct value instead of the cache wrapper

andersfylling commented 3 years ago

I am now very tempted to just make a sync.Map cache as the default.