Anonymous | Login
Project:
RSS
  
News | My View | View Issues | Roadmap | Summary

View Issue Details[ Jump to Notes ] [ Issue History ] [ Print ]
ID
0035754
TypeCategorySeverityReproducibilityDate SubmittedLast Update
defect[Retail Modules] StoreServermajorhave not tried2017-04-11 15:182017-05-01 17:25
ReporteralostaleView Statuspublic 
Assigned ToAugustoMauch 
PrioritynormalResolutionfixedFixed in VersionRR17Q3
StatusclosedFix in branchFixed in SCM revision743d566c0db4
ProjectionnoneETAnoneTarget Version
OSAnyDatabaseAnyJava version
OS VersionDatabase versionAnt version
Product VersionSCM revision 
Review Assigned Tomtaal
Regression level
Regression date
Regression introduced in release
Regression introduced by commit
Triggers an Emergency PackNo
Summary

0035754: MobileServerController.getThisServerDefinition creates contention

DescriptionMobileServerController.getThisServerDefinition method is invoked all the time from many different places. It is synchronized on a singleton class, which in practice, means there is only thread per JVM that can execute it.

This method causes some contention at JVM level.

The situation can become worse in scenarios with overload, if number of DB connections is limited at pool level. Because in some cases, the connection for the thread is borrowed from pool inside this method, this can cause deadlock situations if:

-pool size=1
-thread 1 acquires the connection anywhere
-thread 2 inside this method tries to get a new connection, it cannot get it till thread 1 returns the one it previously borrowed
-thread 1 invokes MobileServerController.getThisServerDefinition
  -> DEADLOCK: it won't be able to execute this method till thread 2 completes it, but it is waiting for thread 1 to release its connection
Steps To ReproduceCan be artificially reproduced by

0. Configure pool as follows:
  db.pool.maxActive=5
  db.pool.maxWait=10000
1. clone https://bitbucket.org/gpscode/butmultiservertests [^]
2. execute wordDay.jmx setting 10 concurrent threads
3. check JVM contention (attached a screenshot from Yourkit)
Proposed Solution* Does it make any sense this method to be synchronized?
  - If it does not: remove syncrhonization
  - If it does: what for? would it properly work in a clustered environment?, in any case look for an alternative

* Additionally, always it's invoked a DB query is triggered, to always return the same record. Could we improve it by some caching?
TagsPerformance
Attached Filespng file icon Selection_166.png [^] (31,040 bytes) 2017-04-11 15:20

- Relationships [ Relation Graph ] [ Dependency Graph ]
depends on backport 0035886RR17Q2.1 closedmtaal Retail Modules MobileServerController.getThisServerDefinition creates contention 
related to defect 0035767 closedmtaal Retail Modules Review synchronized methods in mobile.core, some of them may not be needed 
related to defect 0035768 closedmtaal Retail Modules Review calls to MobileServerController.getThisServerDefinition 
related to defect 0035855 closedalostale Openbravo ERP many standard requests borrow more than one connection from DB 

-  Notes
(0095987)
AugustoMauch (manager)
2017-04-11 16:09

 More than 50% of the methods that invoke MobileServerController.getThisServerDefinition only check if the object itself is null, or check properties that are not going to change (client id, mobile server key, server type, etc). This invokation should not require a query, and could benefit from just getting a cached object.
(0096022)
hgbot (developer)
2017-04-12 12:28

 Repository: erp/pmods/org.openbravo.mobile.core
Changeset: 743d566c0db42fa8378d8d7b63dfee765c62fcce
Author: Augusto Mauch <augusto.mauch <at> openbravo.com>
Date: Wed Apr 12 11:29:47 2017 +0200
URL: http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/743d566c0db42fa8378d8d7b63dfee765c62fcce [^]

Fixes issue 35754: getThisServerDefinition should not be synchronized

This changeset [1] added the synchronized clause to the MobileServerController.getThisServerDefinition method, to prevent a lazy initialization being done more than once with concurrent accesses. This other changeset [2] removed the lazy initialization of the mobile server, so the synchronized clause is no longer needed.

Another issue will be created to provide a method that returns a cached version of the mobile server, to prevent the query to the db when the user only needs to check properties of the mobile server that are not modified.

[1] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/0e660a087cb3#l1.35 [^]
[2] https://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/diff/6a6f980e374a/src/org/openbravo/mobile/core/servercontroller/MobileServerController.java#l1.71 [^]

---
M src/org/openbravo/mobile/core/servercontroller/MobileServerController.java
---
(0096284)
mtaal (manager)
2017-05-01 17:24

 re-open to schedule a backport
(0096285)
mtaal (manager)
2017-05-01 17:25

 resolved
(0096286)
mtaal (manager)
2017-05-01 17:25

 reviewed

- Issue History
Date Modified Username Field Change
2017-04-11 15:18 alostale New Issue
2017-04-11 15:18 alostale Assigned To => StoreServer
2017-04-11 15:18 alostale Triggers an Emergency Pack => No
2017-04-11 15:18 alostale Tag Attached: Performance
2017-04-11 15:20 alostale File Added: Selection_166.png
2017-04-11 15:23 alostale Issue Monitored: alostale
2017-04-11 16:09 AugustoMauch Note Added: 0095987
2017-04-12 10:57 AugustoMauch Assigned To StoreServer => AugustoMauch
2017-04-12 12:13 AugustoMauch Relationship added related to 0035767
2017-04-12 12:26 AugustoMauch Relationship added related to 0035768
2017-04-12 12:28 hgbot Checkin
2017-04-12 12:28 hgbot Note Added: 0096022
2017-04-12 12:28 hgbot Status new => resolved
2017-04-12 12:28 hgbot Resolution open => fixed
2017-04-12 12:28 hgbot Fixed in SCM revision => http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/743d566c0db42fa8378d8d7b63dfee765c62fcce [^]
2017-04-12 12:30 AugustoMauch Review Assigned To => mtaal
2017-04-27 10:00 alostale Relationship added related to 0035855
2017-05-01 17:24 mtaal Note Added: 0096284
2017-05-01 17:24 mtaal Status resolved => new
2017-05-01 17:24 mtaal Resolution fixed => open
2017-05-01 17:24 mtaal Status new => scheduled
2017-05-01 17:25 mtaal Note Added: 0096285
2017-05-01 17:25 mtaal Status scheduled => resolved
2017-05-01 17:25 mtaal Fixed in Version => RR17Q3
2017-05-01 17:25 mtaal Resolution open => fixed
2017-05-01 17:25 mtaal Note Added: 0096286
2017-05-01 17:25 mtaal Status resolved => closed

Copyright © 2000 - 2009 MantisBT Group
Powered by Mantis Bugtracker