Openbravo Issue Tracking System - Retail Modules
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0035754Retail ModulesStoreServerpublic2017-04-11 15:182017-05-01 17:25
Reporteralostale 
Assigned ToAugustoMauch 
PrioritynormalSeveritymajorReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOS5OS Version
Product Version 
Target VersionFixed in VersionRR17Q3 
Merge Request Status
Review Assigned Tomtaal
OBNetwork customer
Support ticket
Regression level
Regression date
Regression introduced in release
Regression introduced by commit
Triggers an Emergency PackNo
Summary0035754: 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?
Additional Information
TagsPerformance
Relationships
depends on backport 0035886RR17Q2.1 closed mtaal Retail Modules MobileServerController.getThisServerDefinition creates contention 
related to defect 0035767 closed mtaal Retail Modules Review synchronized methods in mobile.core, some of them may not be needed 
related to defect 0035768 closed mtaal Retail Modules Review calls to MobileServerController.getThisServerDefinition 
related to defect 0035855 closed alostale Openbravo ERP many standard requests borrow more than one connection from DB 
Attached Filespng Selection_166.png (31,040) 2017-04-11 15:20
https://issues.openbravo.com/file_download.php?file_id=10666&type=bug
png
Issue History
Date ModifiedUsernameFieldChange
2017-04-11 15:18alostaleNew Issue
2017-04-11 15:18alostaleAssigned To => StoreServer
2017-04-11 15:18alostaleTriggers an Emergency Pack => No
2017-04-11 15:18alostaleTag Attached: Performance
2017-04-11 15:20alostaleFile Added: Selection_166.png
2017-04-11 15:23alostaleIssue Monitored: alostale
2017-04-11 16:09AugustoMauchNote Added: 0095987
2017-04-12 10:57AugustoMauchAssigned ToStoreServer => AugustoMauch
2017-04-12 12:13AugustoMauchRelationship addedrelated to 0035767
2017-04-12 12:26AugustoMauchRelationship addedrelated to 0035768
2017-04-12 12:28hgbotCheckin
2017-04-12 12:28hgbotNote Added: 0096022
2017-04-12 12:28hgbotStatusnew => resolved
2017-04-12 12:28hgbotResolutionopen => fixed
2017-04-12 12:28hgbotFixed in SCM revision => http://code.openbravo.com/erp/pmods/org.openbravo.mobile.core/rev/743d566c0db42fa8378d8d7b63dfee765c62fcce [^]
2017-04-12 12:30AugustoMauchReview Assigned To => mtaal
2017-04-27 10:00alostaleRelationship addedrelated to 0035855
2017-05-01 17:24mtaalNote Added: 0096284
2017-05-01 17:24mtaalStatusresolved => new
2017-05-01 17:24mtaalResolutionfixed => open
2017-05-01 17:24mtaalStatusnew => scheduled
2017-05-01 17:25mtaalNote Added: 0096285
2017-05-01 17:25mtaalStatusscheduled => resolved
2017-05-01 17:25mtaalFixed in Version => RR17Q3
2017-05-01 17:25mtaalResolutionopen => fixed
2017-05-01 17:25mtaalNote Added: 0096286
2017-05-01 17:25mtaalStatusresolved => closed

Notes
(0095987)
AugustoMauch   
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   
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   
2017-05-01 17:24   
re-open to schedule a backport
(0096285)
mtaal   
2017-05-01 17:25   
resolved
(0096286)
mtaal   
2017-05-01 17:25   
reviewed