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

Duplicate keys created due to Random class used in FirebaseKeyGenerator not being thread safe #325

Open
gapspt opened this issue Sep 13, 2024 · 0 comments
Labels

Comments

@gapspt
Copy link

gapspt commented Sep 13, 2024

This line is VERY problematic, since Random is not thread safe:

private static readonly Random random = new Random();

This is the reason why:

on a multi-core machine it’s likely that [...] rand.Next() will always return 0. Not very random, is it? This is due to an implementation detail of Random that does not tolerate multithreaded access (it wasn’t designed for it, and the docs explicitly call out that Random should not be used from multiple threads, as they do for most types in the .NET Framework).

Sources:
http://web.archive.org/web/20160326010328/http://blogs.msdn.com/b/pfxteam/archive/2009/02/19/9434171.aspx
https://stackoverflow.com/a/11109361/2024921

Due to this, the ID generation fails spectacularly.

Pleas note that this is not just some theoretical problem, it's very real: In my case this bug is almost always reproduced by just spawning 4 threads at the same time and make one request in each.

I found this bug because I noticed that when adding new objects to a path, the returned keys would be the same.
Tracking down the bug, I realized that there is a generateKeyOffline that can be passed to the PostAsync mothods (referenced below) that will cause the remote database to generate the ID istead of generating it locally.
Calling those methods with false resolves my issue, but the bug persists for other unaware users.

PostAsync methods:

public async Task<FirebaseObject<string>> PostAsync(string data, bool generateKeyOffline = true, TimeSpan? timeout = null)

public static async Task<FirebaseObject<T>> PostAsync<T>(this FirebaseQuery query, T obj, bool generateKeyOffline = true)

In conclusion, besides fixing the core bug, I think it would also be prudent to change the generateKeyOffline default value to false (I can't even see any advantage to generating it offline).

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

No branches or pull requests

2 participants