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

Improve locks in domains and tags tables when short URLs are created #953

Closed
acelaya opened this issue Jan 2, 2021 · 7 comments · Fixed by #1789
Closed

Improve locks in domains and tags tables when short URLs are created #953

acelaya opened this issue Jan 2, 2021 · 7 comments · Fixed by #1789
Labels
Milestone

Comments

@acelaya
Copy link
Member

acelaya commented Jan 2, 2021

Currently, when a short URL is created, it can be linked to a domain and/or list of tags. Those two are created if they don't exist.

However, if two short URLs are tried to be created linked to new tags/domains, both requests run into a race condition that can end up in a fatal error.

The tags/domains tables should be locked by doing a select for update query when checking if they exist, so that these race conditions do not happen.

Once the new entities have been created, the lock can be dropped, unlocking other requests that are waiting.


UPDATE: After some investigation, this task is turning harder than expected. SELECT ... FOR UPDATE does not set the locks properly when the entries do not exist (yes, it's not called SELECT ... FOR INSERT for a reason).

That means the lock would need to be set using an external approach, instead of the native database mechanism.

This article explain the problem in detail: https://mysqlquicksand.wordpress.com/2019/12/20/select-for-update-on-non-existent-rows/

@acelaya acelaya added the bug label Jan 2, 2021
@acelaya acelaya added this to the 2.5.1 milestone Jan 2, 2021
@acelaya acelaya modified the milestones: 2.5.1, 2.5.2, 2.6.0 Jan 21, 2021
@acelaya
Copy link
Member Author

acelaya commented Jan 30, 2021

Blocked by #862

@acelaya acelaya added blocked Issues with some external dependency preventing to work on them and removed blocked Issues with some external dependency preventing to work on them labels Jan 30, 2021
@acelaya acelaya modified the milestones: 2.6.0, 2.7.0 Feb 13, 2021
@acelaya
Copy link
Member Author

acelaya commented Apr 10, 2021

Actually, the issue described in #835 (comment) is probably not caused by a missing locking, but probably by the fact that imported urls are flushed in batches.

If during the same batch the same tag/domain is used for several urls and it didn't exist yet, it will try to be created every time, causing the exception when the flush happens.

In order to solve this, we need to track tags and domains somehow, so that the same references are resolved during the batch.

The locking is also needed, but that will solve a different issue.

@antwonw
Copy link

antwonw commented May 7, 2021

So, I believe this is the fatal error you're referring to?

When importing and the domain is not otherwise known/specified in Shlink:

In AbstractSQLiteDriver.php line 48:
                                                                                                               
  An exception occurred while executing 'INSERT INTO domains (authority) VALUES (?)' with params ["glnr.in"]:  
                                                                                                               
  SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: domains.authority              
                                                                                                               

In Exception.php line 18:
                                                                                                   
  SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: domains.authority  
                                                                                                   

In PDOStatement.php line 115:
                                                                                                   
  SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: domains.authority  

When importing and the tag is not otherwise known/specified in Shlink:

In AbstractSQLiteDriver.php line 48:
                                                                                                         
  An exception occurred while executing 'INSERT INTO tags (name) VALUES (?)' with params ["instagram"]:  
                                                                                                         
  SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: tags.name                
                                                                                                         

In Exception.php line 18:
                                                                                           
  SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: tags.name  
                                                                                           

In PDOStatement.php line 115:
                                                                                           
  SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: tags.name  
                                                                                           

@acelaya
Copy link
Member Author

acelaya commented May 7, 2021

Yes, but that has already been fixed here #1068

This ticket is for something else.

@acelaya acelaya removed this from the 2.7.0 milestone May 23, 2021
@acelaya acelaya added this to Shlink Sep 25, 2022
@acelaya acelaya moved this to Todo 🗒️ in Shlink Sep 25, 2022
@acelaya acelaya removed the status in Shlink Sep 25, 2022
@P1erreGaultier
Copy link

Hello, I am trying Shlink and I experience this issue.
I use 3 shlink instances with one Postgres database and a redis to shared locks.

When I try to create two links with a tag that doesn't exist (two calls to /short-urls endpoint at the same time), it fails with database unicity constraint error.

Since this issue is not resolved, I managed to avoid this situation as much as possible, but I still have errors sometime.
Do you know if there is a workaround to prevent error on tags creation ?

Thank you for your work on Shlink !

Pierre

@acelaya
Copy link
Member Author

acelaya commented Mar 22, 2023

Nop, nothing to do for now until this is fixed.

It shouldn't happen too often, unless you have a heavy short URL creation load.

@acelaya acelaya added this to the 3.6.0 milestone Apr 16, 2023
@acelaya acelaya moved this to Todo in Shlink Apr 16, 2023
@acelaya acelaya moved this from Todo to In Progress in Shlink May 21, 2023
@acelaya acelaya moved this from In Progress to In review in Shlink May 21, 2023
@github-project-automation github-project-automation bot moved this from In review to Done in Shlink May 21, 2023
@acelaya
Copy link
Member Author

acelaya commented May 21, 2023

Shlink now includes its own external locks to make sure more than one short URL being created in parallel with the same new domain and/or tag/s, does not result in a duplicated key error.

This will be shipped with Shlink 3.6.0, and will require redis to properly work on a multi-instance set-up.

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

Successfully merging a pull request may close this issue.

3 participants