Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise on id mismatch from the cache #327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

slemiere
Copy link

Hello,

I recently found an issue on an application of mine that was leading to cache corruption. When that happened I was able to see very helpful lines like:

[IDC id mismatch] fetch_by_id_requested=248053 fetch_by_id_got=390397 for #<User id: 390397, email: ...

For my use case when this happen it actually make more sense to raise an exception to prevent the transaction from finishing with the wrong user. Would you consider adding a configuration option to enable this behavior ?

Thank you

@dylanahsmith
Copy link
Contributor

For my use case when this happen it actually make more sense to raise an exception to prevent the transaction from finishing with the wrong user.

I would recommend not using IDC for a transaction. This is why we mark records returned by IDC as read-only, since we don't want to persist cache corruption. IdentityCache also detects when it is in a transaction and avoids trying to get the record from the cache and just gets it directly from the database.

@slemiere
Copy link
Author

The problem is not really that I will persist the object to the database but more that I will act on the record based on the wrong information. For instance if I am fetching a user_id from the cache in order to send an email to that user I would end up sending an email to the wrong user.

@slemiere
Copy link
Author

slemiere commented May 4, 2017

@dylanahsmith do you have any feedback on the above ? Thanks a lot !

@dylanahsmith
Copy link
Contributor

I'm not sure when this would actually happen. Normally I would expect cache corruption to just result in stale data being in the cache. However, I'm not sure why there would be an id mismatch, since the cache key contains the id.

Do you have any insights on what type of cache corruption can lead to this error?

I'm not opposed to changing the code to raise when this error occurs without a configuration option for this behaviour.

@slemiere
Copy link
Author

slemiere commented May 4, 2017

For us this happened because of a mismatch between uuid and integer ids. Basically we were calling the fetch method with a device UUID (something like 9d2e4c7c-cb79-41ad-9c14-3a805f3d23a9) when it was supposed to be an integer ID. It would then be casted to integer and the DB was happy to return the record for device with id 9 (the start of that UUID) which was the wrong device.

@slemiere
Copy link
Author

slemiere commented May 4, 2017

I'm happy to make the necessary changes in that PR to make the raising behavior happen without the configuration option.

@dylanahsmith
Copy link
Contributor

Thanks for the insight. I think we could probably check for that in the IdentityCache.fetch block in order to raise an exception before the data is put in the cache in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants