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.

Sign up for GitHub

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

Jump to bottom

[JBPM-10187] Sorting resources to avoid deadlock #2318

Merged
merged 6 commits into from
Nov 6, 2023
Merged

[JBPM-10187] Sorting resources to avoid deadlock #2318

merged 6 commits into from
Nov 6, 2023

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Aug 17, 2023 •
edited
Loading

JIRA:

link

referenced Pull Requests:
kiegroup/drools#11

@fjtirado fjtirado closed this Aug 17, 2023
@fjtirado fjtirado reopened this Aug 17, 2023
@fjtirado fjtirado force-pushed the JBPM-10187 branch 2 times, most recently from cbaf116 to 33a3fb7 Compare August 17, 2023 07:48
@fjtirado fjtirado changed the title Jbpm 10187 [JBPM-10187] Sorting resources to avoid deadlock Aug 17, 2023
@fjtirado fjtirado force-pushed the JBPM-10187 branch 3 times, most recently from fa432de to 4a2eb20 Compare August 21, 2023 11:51
@fjtirado fjtirado force-pushed the JBPM-10187 branch 2 times, most recently from 8554120 to 118fc9a Compare September 18, 2023 16:59
@fjtirado fjtirado force-pushed the JBPM-10187 branch 2 times, most recently from 92c78c6 to 533d178 Compare September 18, 2023 17:20
@sonarcloud
Copy link

sonarcloud bot commented Sep 18, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

61.3% 61.3% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@fjtirado fjtirado mentioned this pull request Sep 19, 2023
Closed
@fjtirado
[JBPM-10187] Using proper SessionNotFoundException
3247732 
Drools is using a different SessionNotFoundException that runtime
manager. We are only interested on the persistence one.
@fjtirado
[JBPM-10187] Fully logging marshaling exception
e3c32e8
@mareknovotny
Copy link
Member

@krisv @albfan is this ok to review as this looks like one fix we would need to review and merge or explain why it is not good change ;)

@mareknovotny
Copy link
Member

of course we need to remove log trace msgs from code

@fjtirado
Copy link
Contributor Author

of course we need to remove log trace msgs from code

why? These traces are legit anyway

@mareknovotny
Copy link
Member

of course we need to remove log trace msgs from code

why? These traces are legit anyway

ok, i thought it is just for debugging purposes. If they are valid even for production, then ok.

elguardian
elguardian reviewed Oct 26, 2023
View reviewed changes
jbpm-runtime-manager/src/main/java/org/jbpm/runtime/manager/impl/AbstractRuntimeManager.java
@@ -389,8 +390,8 @@ protected boolean canDispose(RuntimeEngine runtime) {
&& tm.getStatus() != TransactionManager.STATUS_COMMITTED) {
return false;
}
} catch (Exception e) {
logger.warn("Exception dealing with transaction", e);
} catch (SessionNotFoundException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not equivalent. lossing info in other exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention of this change is to let the transaction fail if the exception is different from SessionNotFoundException, but to be honest I do not recall why I though that will be useful for this particular issue.
So, I guess the question is, does it make sense to hide all exceptions here or just the SesisonNotFound?

@elguardian
Copy link
Member

TLGTM without going too deep into the details.
Just to double check:
You are catching a problem in the RuntimeManager where two members in cluster are operating over the same process instance causing a race condition and one tx is deleting the process instance ? @fjtirado

@gmunozfe
Copy link
Member

jenkins retest this

@fjtirado fjtirado marked this pull request as ready for review October 26, 2023 14:36
gmunozfe
gmunozfe reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@gmunozfe gmunozfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments about tracing, possible null pointers and skipping supresswarnings)

...ce/jbpm-persistence-jpa/src/main/java/org/jbpm/persistence/JpaProcessPersistenceContext.java Show resolved Hide resolved
...me-manager/src/main/java/org/jbpm/runtime/manager/impl/PerProcessInstanceRuntimeManager.java Show resolved Hide resolved
...me-manager/src/main/java/org/jbpm/runtime/manager/impl/PerProcessInstanceRuntimeManager.java Show resolved Hide resolved
...me-manager/src/main/java/org/jbpm/runtime/manager/impl/PerProcessInstanceRuntimeManager.java Show resolved Hide resolved
...me-manager/src/main/java/org/jbpm/runtime/manager/impl/PerProcessInstanceRuntimeManager.java Show resolved Hide resolved
@fjtirado fjtirado mentioned this pull request Oct 26, 2023
Merged
@mareknovotny
Copy link
Member

jenkins retest this please

@fjtirado @gmunozfe
[JBPM-10187] Gonzalo comments
f21a8d7 
Co-authored-by: Gonzalo Muñoz <[email protected]>
@fjtirado fjtirado added backport-7.67.x Generate backport PR for 7.67.x branch backport-7.67.x-blue Generate backport PR for 7.67.x-blue branch labels Oct 26, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

61.0% 61.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

gmunozfe
gmunozfe approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@gmunozfe gmunozfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, excellent work @fjtirado !

@fjtirado fjtirado merged commit 2a809ac into kiegroup:main Nov 6, 2023
8 of 9 checks passed
This was referenced Nov 6, 2023
Merged
Merged
Merged
github-actions bot pushed a commit that referenced this pull request Nov 6, 2023
@fjtirado @gmunozfe
[JBPM-10187] Sorting resources to avoid deadlock (#2318)
e456f1e 
* [JBPM-10187] Adding traces for process instance info

* [JBPM-10187] Propagate exceptions different than SessionNotFound

* [JBPM-10187] Handling sessionnotfound in local thread

* [JBPM-10187] Using proper SessionNotFoundException

Drools is using a different SessionNotFoundException that runtime
manager. We are only interested on the persistence one.

* [JBPM-10187] Fully logging marshaling exception

* [JBPM-10187] Gonzalo comments

Co-authored-by: Gonzalo Muñoz <[email protected]>

---------

Co-authored-by: Gonzalo Muñoz <[email protected]>
github-actions bot pushed a commit that referenced this pull request Nov 6, 2023
@fjtirado @gmunozfe
[JBPM-10187] Sorting resources to avoid deadlock (#2318)
422d712 
* [JBPM-10187] Adding traces for process instance info

* [JBPM-10187] Propagate exceptions different than SessionNotFound

* [JBPM-10187] Handling sessionnotfound in local thread

* [JBPM-10187] Using proper SessionNotFoundException

Drools is using a different SessionNotFoundException that runtime
manager. We are only interested on the persistence one.

* [JBPM-10187] Fully logging marshaling exception

* [JBPM-10187] Gonzalo comments

Co-authored-by: Gonzalo Muñoz <[email protected]>

---------

Co-authored-by: Gonzalo Muñoz <[email protected]>
@github-actions github-actions bot mentioned this pull request Nov 6, 2023
Merged
fjtirado added a commit to fjtirado/jbpm that referenced this pull request Nov 7, 2023
@fjtirado @gmunozfe
[JBPM-10187] Sorting resources to avoid deadlock (kiegroup#2318)
668d5a4 
* [JBPM-10187] Adding traces for process instance info

* [JBPM-10187] Propagate exceptions different than SessionNotFound

* [JBPM-10187] Handling sessionnotfound in local thread

* [JBPM-10187] Using proper SessionNotFoundException

Drools is using a different SessionNotFoundException that runtime
manager. We are only interested on the persistence one.

* [JBPM-10187] Fully logging marshaling exception

* [JBPM-10187] Gonzalo comments

Co-authored-by: Gonzalo Muñoz <[email protected]>

---------

Co-authored-by: Gonzalo Muñoz <[email protected]>
fjtirado added a commit that referenced this pull request Nov 17, 2023
@github-actions @fjtirado @gmunozfe
[JBPM-10187] Sorting resources to avoid deadlock (#2318) (#2357)
1a7fb8b 
* [JBPM-10187] Adding traces for process instance info

* [JBPM-10187] Propagate exceptions different than SessionNotFound

* [JBPM-10187] Handling sessionnotfound in local thread

* [JBPM-10187] Using proper SessionNotFoundException

Drools is using a different SessionNotFoundException that runtime
manager. We are only interested on the persistence one.

* [JBPM-10187] Fully logging marshaling exception

* [JBPM-10187] Gonzalo comments



---------

Co-authored-by: Francisco Javier Tirado Sarti <[email protected]>
Co-authored-by: Gonzalo Muñoz <[email protected]>
fjtirado added a commit that referenced this pull request Nov 17, 2023
@github-actions @fjtirado @gmunozfe
[JBPM-10187] Sorting resources to avoid deadlock (#2318) (#2356)
841e602 
* [JBPM-10187] Adding traces for process instance info

* [JBPM-10187] Propagate exceptions different than SessionNotFound

* [JBPM-10187] Handling sessionnotfound in local thread

* [JBPM-10187] Using proper SessionNotFoundException

Drools is using a different SessionNotFoundException that runtime
manager. We are only interested on the persistence one.

* [JBPM-10187] Fully logging marshaling exception

* [JBPM-10187] Gonzalo comments



---------

Co-authored-by: Francisco Javier Tirado Sarti <[email protected]>
Co-authored-by: Gonzalo Muñoz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elguardian elguardian elguardian left review comments

@mareknovotny mareknovotny mareknovotny left review comments

@baldimir baldimir baldimir left review comments

@gmunozfe gmunozfe gmunozfe approved these changes

@krisv krisv Awaiting requested review from krisv

@martinweiler martinweiler Awaiting requested review from martinweiler

Assignees
No one assigned
Labels
backport-7.67.x Generate backport PR for 7.67.x branch backport-7.67.x-blue Generate backport PR for 7.67.x-blue branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fjtirado @mareknovotny @elguardian @gmunozfe @baldimir