ash-jc-allen
commented
1 month ago Hey @WilliamVenner, thanks for the PR!
I'm really sorry that you ran into problems with the package in production. When I first built this package, I only ever imagined that I'd use it for generating a few short URLs (100 at most). I never really thought that other devs would want to use it or attempt to generate a lot of short URLs. So scalability wasn't something I paid a huge amount of attention to!
For most use cases, the default class that ships for generating short URLs will do the trick. But for larger apps like yours, you typically need something a bit more performant and bespoke to your application.
So I think I'll close this PR for the time being, sorry. But if it's something people think would be useful, then I'll consider merging this in.
You might already know, but you can override the key generator (https://github.com/ash-jc-allen/short-url?tab=readme-ov-file#specifying-the-key-generator). It might be useful to create your own key generation logic that fits your application's needs?
Huge thanks again for the contribution though, I really appreciate the time and effort! 😄
Hello,
In production we came across a pretty nasty bug where we were experiencing a huge amount of collisions when trying to generate a new shortlink ID:
As you can see, it had to make almost 1,000 database queries before it could insert a shortlink into the database.
Evidently, we're generating a lot of shortlinks in a small space of time, which is causing a ton of collisions.
Investigating further, we found that it was stuck in this loop:
https://github.com/ash-jc-allen/short-url/blob/6bcc4850d1cc62f957cf2f94941f196498d20ab8/src/Classes/KeyGenerator.php#L35-L38
The bug is that this loop isn't atomic: by the time we exit the loop or check the loop condition, another shortlink might have been inserted in place of ours before we could save our own new shortlink into the database.
In this PR I have swapped the key generation implementation with something that "reserves" a row in the database first, before generating a URL key. This makes it atomic.
The implementation itself is a bit hacky, but the idea is to maintain backwards compatibility as much as possible: the only actionable change is that a new migration needs to be run.