RobThree / IdGen

Twitter Snowflake-alike ID generator for .Net
MIT License
1.19k stars 149 forks source link

Thread safety #33

Closed Maslzad closed 3 years ago

Maslzad commented 3 years ago

Hi, on too many requests, the IdGen package has a duplicate ID problem, and some requests fail. You seem to have forgotten to define the static lock. Is shown in the image below. image

RobThree commented 3 years ago

Do you actually have or experience a problem? Or do you think a locking object needs to be static? Can you provide a testcase?

Are you using the IdGenerator as singleton?

Maslzad commented 3 years ago

Yes, I have a test case with distributed JMeter with 500 threads, Every 10 requests received an error. (duplicate primary key in the database). after the mentioned change, I no longer had a problem using IdGen. you could read Jon skeet singleton pattern thread-safety article, explain how to make thread-safe with private readonly static object

https://csharpindepth.com/articles/singleton

RobThree commented 3 years ago

You haven't answered my question:

Are you using the IdGenerator as singleton?

Or have you registered it as scoped? Because the IdGenerator is intended to be used as singleton.

Maslzad commented 3 years ago

I registered as a singleton.

RobThree commented 3 years ago

Then please provide your testcase so I can (try to) reproduce.

RobThree commented 3 years ago

Ok, I have tested this with up to 100,000 requests with 500 concurrent threads . Not a single duplicate. Here's my test-setup:

Test.zip

Open the solution and run the project (make sure you run in Release mode!!). This will start a service that has a single controller that has the following methods:

Also included is a bench.cmd file that shows how I run ab.exe (Apache Benchmark). The easiest way to obtain ab.exe (a small executable of just 96 kB) is to download XAMPP Portable - ZIP (189 MB) and extract the zipfile and copy the ab.exe file from xampp/apache/bin to the test directory where the bench.cmd also resides. Now, with the webserver up, run bench.cmd.

Note the output:

> ab.exe -n 100000 -c 500 http://localhost:5000/test/getid
This is ApacheBench, Version 2.3 <$Revision: 1879490 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 10000 requests
Completed 20000 requests
Completed 30000 requests
Completed 40000 requests
Completed 50000 requests
Completed 60000 requests
Completed 70000 requests
Completed 80000 requests
Completed 90000 requests
Completed 100000 requests
Finished 100000 requests

Server Software:        Kestrel
Server Hostname:        localhost
Server Port:            5000

Document Path:          /test/getid
Document Length:        18 bytes

Concurrency Level:      500
Time taken for tests:   24.431 seconds
Complete requests:      100000
Failed requests:        0
Total transferred:      15700000 bytes
HTML transferred:       1800000 bytes
Requests per second:    4093.18 [#/sec] (mean)
Time per request:       122.155 [ms] (mean)
Time per request:       0.244 [ms] (mean, across all concurrent requests)
Transfer rate:          627.57 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.4      0       2
Processing:    31  122  33.3    127     195
Waiting:        2   61  38.4     55     179
Total:         31  122  33.4    127     196

Percentage of the requests served within a certain time (ms)
  50%    127
  66%    143
  75%    150
  80%    153
  90%    163
  95%    171
  98%    177
  99%    180
 100%    196 (longest request)

On my Intel Core i9-10900X (10 cores, 20 threads) this runs perfectly fine (note the "Failed requests: 0". If any requests fail, the line below Failed requests: will read something like Non-2xx responses: so you need to be especially on the lookout for that. You can easily test this by changing line 25 in TestController.cs to var id = 12345; so all Id's will be a duplicate. This should result in a 400 Bad Request and be counted as a Non-2xx response and will also add the value 12345 a single time to the dictionary, to /test/count will return 1.

After you ran a bench you can also check out http://localhost:5000/test/count which should have a cumulative count of all the times /test/getid was invoked.

I haven't seen a single duplicate in a LOT of runs. So, please, provide me with your testcase so I can run yours.

RobThree commented 3 years ago

Oh, I forgot to mention:

As soon as I change line 31 in program.cs from:

services.AddSingleton<IIdGenerator<long>>(c => new IdGenerator(0));

to:

services.AddScoped<IIdGenerator<long>>(c => new IdGenerator(0));

I see the numbers change and about 74,040 of 100,000 (74%) of the calls fail (when in Release mode).

    > ab.exe -n 100000 -c 500 http://localhost:5000/test/getid
    This is ApacheBench, Version 2.3 <$Revision: 1879490 $>
    Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
    Licensed to The Apache Software Foundation, http://www.apache.org/

    Benchmarking localhost (be patient)
    Completed 10000 requests
    Completed 20000 requests
    Completed 30000 requests
    Completed 40000 requests
    Completed 50000 requests
    Completed 60000 requests
    Completed 70000 requests
    Completed 80000 requests
    Completed 90000 requests
    Completed 100000 requests
    Finished 100000 requests

    Server Software:        Kestrel
    Server Hostname:        localhost
    Server Port:            5000

    Document Path:          /test/getid
    Document Length:        18 bytes

    Concurrency Level:      500
    Time taken for tests:   26.686 seconds
    Complete requests:      100000
    Failed requests:        74040
       (Connect: 0, Receive: 0, Length: 74040, Exceptions: 0)
    Non-2xx responses:      74040
    Total transferred:      15625960 bytes
    HTML transferred:       1503840 bytes
    Requests per second:    3747.28 [#/sec] (mean)
    Time per request:       133.430 [ms] (mean)
    Time per request:       0.267 [ms] (mean, across all concurrent requests)
    Transfer rate:          571.83 [Kbytes/sec] received

    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.4      0       2
    Processing:    29  133  33.1    142     198
    Waiting:        2   68  41.8     62     196
    Total:         30  133  33.2    142     198

    Percentage of the requests served within a certain time (ms)
      50%    142
      66%    152
      75%    158
      80%    162
      90%    170
      95%    176
      98%    186
      99%    190
     100%    198 (longest request)
Maslzad commented 3 years ago

I tested your code with your test plan and with mine, I didn't see any duplicated Id, Everything went well !! here is my registration in production code image and LongIdenetityGenerator class image is something wrong with registration and implemented class

RobThree commented 3 years ago

Will you please post code as text next time instead of screenshots? Now I need to manually type this all in by hand like some caveman... 😬

Also you left out the IIdentitySettingsProvider interface, the IIdentitySettingsProvider interface and a class implementing IIdentitySettingsProvider I had to guess. However, I have tried to copy/implement what you have and this, again, seems to work fine:

Test.zip

> ab.exe -n 100000 -c 500 http://localhost:5000/test/getid
...
Complete requests:      100000
Failed requests:        0
...

Please provide me with an actual testcase, be it a zipfile, a git repository, something, so I can have a look at your actual (test)code and try to reproduce the issue.

RobThree commented 3 years ago

@Maslzad Could you please provide your testcase? Zipfile or link or anything will do. I would really love to figure out what is going on.

RobThree commented 3 years ago

@Maslzad ??

RobThree commented 3 years ago

If you ever decide to get back to me @Maslzad that would be nice.