lukas-krecan / ShedLock

Distributed lock for your scheduled tasks
Apache License 2.0
3.66k stars 518 forks source link

Propagation Behavior defaulting to REQUIRES_NEW causing transaction issues. #1076

Open cday2 opened 2 years ago

cday2 commented 2 years ago

Is your feature request related to a problem? Please describe. Yes, because you are doing transactionTemplate.setPropagationBehavior to REQUIRES_NEW in JdbcTemplateStorageAccessor constructor we have code that has an @Transactional and does not update the DB in certain cases.

Describe the solution you'd like Make it configurable via the Configuration. We needed PROPAGATION_REQUIRED to avoid transaction issues.

Describe alternatives you've considered Maybe even rethink it and change it to PROPAGATION_REQUIRED I bet others run into this issue. We are using your shedlock not just for scheduled jobs but for system level locks.

Additional context

image

lukas-krecan commented 2 years ago

Hi, thanks for feedback. Feel free to send a pull request. Out of curiosity, can you please elaborate on the issues it causes?

cday2 commented 2 years ago

Exactly, something like that but for propgationBehavior.

We are using Sherlock for system wide locks as well as for @Scheduled locks.

We have an use case where we need to update the DB and also do a system wide lock within our own transaction boundary. With TransactionDefinition.PROPAGATION_REQUIRES_NEW it creates another transaction and doesn’t work in some code paths with our transaction boundaries. The result was our own transaction db update doesn’t get made. When we change it to TransactionDefinition.PROPAGATION_REQUIRED it works fine.

I think TransactionDefinition.PROPAGATION_REQUIRES_NEW works well for using it on @Scheduled annotations but has some weaknesses when you use it for other things like we are with transaction boundaries. You might want to consider even changing the default to TransactionDefinition.PROPAGATION_REQUIRES_NEW. It will still create a transaction if it is not there and join the transaction boundary if not.

We are using mysql with spring boot transaction manager.

Found this in stack overflow: on REQUIRES_NEW if you are interested. https://stackoverflow.com/questions/5789788/what-exactly-propagation-requires-new-means-using-spring-transaction-managemen https://stackoverflow.com/questions/5789788/what-exactly-propagation-requires-new-means-using-spring-transaction-managemen

BTW, other than this issue Sherlock has worked great for us. Thanks for offering such a great open source product and supporting it.

On Jun 22, 2022, at 11:30 AM, Lukáš Křečan @.***> wrote:

Hi, thanks for feedback. Feel free to send a pull request. Out of curiosity, can you please elaborate on the issues it causes?

— Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_lukas-2Dkrecan_ShedLock_issues_1076-23issuecomment-2D1163264283&d=DwMCaQ&c=NxS7LVD4EucgUR9_G6bWzuqhmQ0xEJ2AZdqjz4WaSHU&r=e5qr6ILf6miGS0mdzRjROzXGrlJY_Udy0HJVjiUi17k&m=p_pAkxLVsqh9DqxKxgmuNhpct5CSt8b0TChcqypwhlYdms4pFx3F84WH04K-8ogr&s=GLEg671nX9OHKpBiUE9EjWEmw3-qEQgdNlTI5cSf2Xs&e=, or unsubscribe https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AVKUPFWMMKRLMLGTX5PAFOTVQMWQHANCNFSM5ZNYGNMQ&d=DwMCaQ&c=NxS7LVD4EucgUR9_G6bWzuqhmQ0xEJ2AZdqjz4WaSHU&r=e5qr6ILf6miGS0mdzRjROzXGrlJY_Udy0HJVjiUi17k&m=p_pAkxLVsqh9DqxKxgmuNhpct5CSt8b0TChcqypwhlYdms4pFx3F84WH04K-8ogr&s=uIOGcv-FxP63i-9wAcaqRIDjE1GW6TK7dWBl2-lAYmk&e=. You are receiving this because you authored the thread.

-- This message may contain confidential and/or privileged information.  If you are not the addressee or authorized to receive this on behalf of the addressee you must not use, copy, disclose or take action based on this message or any information herein.  If you have received this message in error, please advise the sender immediately by reply email and delete this message. Thank you.

cday2 commented 2 years ago

Meant to say: You might want to consider even changing the default to TransactionDefinition.PROPAGATION_REQUIRES_NEW.PROPAGATION_REQUIRED

On Wed, Jun 22, 2022 at 11:43 AM Carson Day @.***> wrote:

Exactly, something like that but for propgationBehavior.

We are using Sherlock for system wide locks as well as for @Scheduled locks.

We have an use case where we need to update the DB and also do a system wide lock within our own transaction boundary. With TransactionDefinition.PROPAGATION_REQUIRES_NEW it creates another transaction and doesn’t work in some code paths with our transaction boundaries. The result was our own transaction db update doesn’t get made. When we change it to TransactionDefinition.PROPAGATION_REQUIRED it works fine.

I think TransactionDefinition.PROPAGATION_REQUIRES_NEW works well for using it on @Scheduled annotations but has some weaknesses when you use it for other things like we are with transaction boundaries. You might want to consider even changing the default to TransactionDefinition.PROPAGATION_REQUIRES_NEW. It will still create a transaction if it is not there and join the transaction boundary if not.

We are using mysql with spring boot transaction manager.

Found this in stack overflow: on REQUIRES_NEW if you are interested.

https://stackoverflow.com/questions/5789788/what-exactly-propagation-requires-new-means-using-spring-transaction-managemen

BTW, other than this issue Sherlock has worked great for us. Thanks for offering such a great open source product and supporting it.

On Jun 22, 2022, at 11:30 AM, Lukáš Křečan @.***> wrote:

Hi, thanks for feedback. Feel free to send a pull request. Out of curiosity, can you please elaborate on the issues it causes?

— Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_lukas-2Dkrecan_ShedLock_issues_1076-23issuecomment-2D1163264283&d=DwMCaQ&c=NxS7LVD4EucgUR9_G6bWzuqhmQ0xEJ2AZdqjz4WaSHU&r=e5qr6ILf6miGS0mdzRjROzXGrlJY_Udy0HJVjiUi17k&m=p_pAkxLVsqh9DqxKxgmuNhpct5CSt8b0TChcqypwhlYdms4pFx3F84WH04K-8ogr&s=GLEg671nX9OHKpBiUE9EjWEmw3-qEQgdNlTI5cSf2Xs&e=, or unsubscribe https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AVKUPFWMMKRLMLGTX5PAFOTVQMWQHANCNFSM5ZNYGNMQ&d=DwMCaQ&c=NxS7LVD4EucgUR9_G6bWzuqhmQ0xEJ2AZdqjz4WaSHU&r=e5qr6ILf6miGS0mdzRjROzXGrlJY_Udy0HJVjiUi17k&m=p_pAkxLVsqh9DqxKxgmuNhpct5CSt8b0TChcqypwhlYdms4pFx3F84WH04K-8ogr&s=uIOGcv-FxP63i-9wAcaqRIDjE1GW6TK7dWBl2-lAYmk&e= . You are receiving this because you authored the thread.Message ID: @.***>

-- Carson Day SDEII T: +1 212 609 XXXX M: +1 404 663 5721 L: 11720 Amber Park Drive Suite 500 Alpharetta, GA 30009 https://www.linkedin.com/company/164748 https://twitter.com/liveperson https://www.facebook.com/liveperson/?ref=bookmarks Our mission is to make life easier by transforming how people communicate with brands. https://www.liveperson.com/resources/news/liveperson-named-one-of-fast-companys-most-innovative-companies/?utm_medium=email&utm_source=gmail&utm_campaign=signature&utm_content=fastco

-- This message may contain confidential and/or privileged information.  If you are not the addressee or authorized to receive this on behalf of the addressee you must not use, copy, disclose or take action based on this message or any information herein.  If you have received this message in error, please advise the sender immediately by reply email and delete this message. Thank you.

lukas-krecan commented 1 year ago

Hi, I am thinking about implementing it. Let's say we have 3 DB operations

  1. ShedLock update to obtain the lock
  2. A DB operation inside the locked method
  3. ShedLock update to release the lock

Currently, thanks to REQUIRES_NEW the operations are isolated. If we change the behavior, an error when executing 2. would rollback 1. Is this what you want? Do you want a failure in ulocking 3. to rollback the DB operation in 2.? What do you expect to happen?