Synchronization issues are at times hard to discover and hard to debug. Therefore, functional testing is unlikely to pick up on this.

Here is a class of problem found by static code review and verified by peer review.

DL: Synchronization on Boolean could lead to deadlock (DL_SYNCHRONIZATION_ON_BOOLEAN)

The code synchronizes on a boxed primitive constant, such as an Boolean.

private static Boolean inited = Boolean.FALSE;
...
  synchronized(inited) {
    if (!inited) {
       init();
       inited = Boolean.TRUE;
       }
     }
...

Since there normally exist only two Boolean objects, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock

We see this in Samigo, org.sakaiproject.tool.assessment.ui.listener.author.PublishAssessmentListener

76 private static Boolean repeatedPublish = Boolean.FALSE;
77
78 public PublishAssessmentListener() {
79 }
80
81 public void processAction(ActionEvent ae) throws AbortProcessingException {
82 synchronized(repeatedPublish)
83 {
84 //FacesContext context = FacesContext.getCurrentInstance();
85
86 UIComponent eventSource = (UIComponent) ae.getSource();
87 ValueBinding vb = eventSource.getValueBinding("value");
88 String buttonValue = (String) vb.getExpressionString();
89 if(buttonValue.endsWith(".button_unique_save_and_publish}"))
90 {
91 repeatedPublish = Boolean.FALSE;
92 return;
93 }
94
95 if(!repeatedPublish.booleanValue())
96 {
97 //Map reqMap = context.getExternalContext().getRequestMap();
98 //Map requestParams = context.getExternalContext().getRequestParameterMap();
99 AuthorBean author = (AuthorBean) ContextUtil.lookupBean(
100 "author");
101
102 AssessmentSettingsBean assessmentSettings = (AssessmentSettingsBean) ContextUtil.lookupBean("assessmentSettings");
103
104 AssessmentService assessmentService = new AssessmentService();
105
106 AssessmentFacade assessment = assessmentService.getAssessment(
107 assessmentSettings.getAssessmentId().toString());
108
109 // 0. sorry need double checking assesmentTitle and everything
110 boolean error = checkTitle(assessment);
111 if (error){
112 return;
113 }
114
115 publish(assessment, assessmentSettings);
116
117 GradingService gradingService = new GradingService();
118 PublishedAssessmentService publishedAssessmentService = new PublishedAssessmentService();
119 AuthorActionListener authorActionListener = new AuthorActionListener();
120 authorActionListener.prepareAssessmentsList(author, assessmentService, gradingService, publishedAssessmentService);
121
122
123 repeatedPublish = Boolean.TRUE;
124 }
125 }
126 }
127
128 private void publish(AssessmentFacade assessment,
129 AssessmentSettingsBean assessmentSettings) {
130 //String publishAssessment = (String) FacesContext.getCurrentInstance().getExternalContext().getRequestParameterMap().get("publishAssessment");
131 //log.info("***** PUBLISHING ***");
132 PublishedAssessmentService publishedAssessmentService = new
133 PublishedAssessmentService();
134 PublishedAssessmentFacade pub = null;
135 try {
136 pub = publishedAssessmentService.publishAssessment(assessment);
137 EventTrackingService.post(EventTrackingService.newEvent("sam.assessment.publish", "assessmentId=" + assessment.getAssessmentId() + ", publishedAssessmentId=" + pub.getPublishedAssessmentId(), true));
138 } catch (AssignmentHasIllegalPointsException gbe) {
139 // Right now gradebook can only accept assessements with totalPoints > 0
140 // this might change later
141 log.warn(gbe);
142 gbe.printStackTrace();
143 // Add a global message (not bound to any component) to the faces context indicating the failure
144 String err=(String)ContextUtil.getLocalizedString("org.sakaiproject.tool.assessment.bundle.AuthorMessages",
145 "gradebook_exception_min_points");
146 FacesContext.getCurrentInstance().addMessage(null, new FacesMessage(err));
147 throw new AbortProcessingException(gbe);
148
149
150 } catch (Exception e) {
151 log.warn(e);
152 e.printStackTrace();
153 // Add a global message (not bound to any component) to the faces context indicating the failure
154 String err=(String)ContextUtil.getLocalizedString("org.sakaiproject.tool.assessment.bundle.AuthorMessages",
155 "gradebook_exception_error");
156 FacesContext.getCurrentInstance().addMessage(null, new FacesMessage(err));
157 throw new AbortProcessingException(e);
158 }
159
160 // let's check if we need a publishedUrl
161 String releaseTo = pub.getAssessmentAccessControl().getReleaseTo();
162 if (releaseTo != null) {
163 // generate an alias to the pub assessment
164 String alias = assessmentSettings.getAlias();
165 PublishedMetaData meta = new PublishedMetaData(pub.getData(),
166 AssessmentMetaDataIfc.ALIAS, alias);
167 publishedAssessmentService.saveOrUpdateMetaData(meta);
168 }
169 }


We see also a similar situation four times in citations, for example the number returned here may not be the expected incremented valu. Gilgamesh says: "the Integer object might be published to other threads before the actual value has been calculated." It would be better to use an java.util.concurrent.AtomicInteger since it guarantees atomicity for this call.

org.sakaiproject.citation.impl.BaseCitationService

5196 protected Integer nextSerialNumber()
5197 {
5198 Integer number;
5199 synchronized (m_nextSerialNumber)
5200 {
5201 number = m_nextSerialNumber;
5202 m_nextSerialNumber = new Integer(number.intValue() + 1);
5203 }
5204
5205 return number;
5206 }


And again in content

org.sakaiproject.content.tool.ResourcesMetadata

1002 protected static String assignAbbrev(String namespace)
1003 {
1004 String abbrev = "error";
1005 synchronized(NamespaceNumber)
1006 {
1007 abbrev = "s" + NamespaceNumber;
1008 NamespaceNumber = new Integer(NamespaceNumber.byteValue() + 1);
1009 }
1010 setNamespaceAbbrev(namespace, abbrev);
1011 return abbrev;
1012 }


8 times the following in Samigo. The lock is being changed inside the synchronzied block so may not be locking.
org.sakaiproject.tool.assessment.qti.util.PathInfo

Example

152 synchronized(this.basePathToSecurity)
153 {
154 this.basePathToSecurity = basePathToSecurity;
155 }


And calendar 2x
org.sakaiproject.calendar.impl.BaseExternalCalendarSubscriptionService$SubscriptionCacheMap


1536 public void setSubscriptionExpiredListener(SubscriptionExpiredListener listener)
1537 {
1538 if(this.listener != null)
1539 {
1540 synchronized(this.listener)
1541 {
1542 this.listener = listener;
1543 }
1544 }
1545 else
1546 this.listener = listener;
1547 }
1548
1549 public void removeSubscriptionExpiredListener()
1550 {
1551 synchronized(this.listener)
1552 {
1553 this.listener = null;
1554 }
1555 }