Details

    • Type: Sub-task
    • Status: CLOSED
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.7.0
    • Fix Version/s: 2.7.0
    • Component/s: Calendar Summary
    • Labels:
      None
    • Environment:
      Staitc code review Sakai 2-7-M2

      Description

      [1,2] org.sakaiproject.calendar.impl.BaseExternalCalendarSubscriptionService
      Line 1603
      Synchronization problems with setSubscriptionExpiredListener and removeSubscriptionExpiredListener()
      Race condition if this.listener changed after testing the condition, but before the synchronized.

      Further, and one I am not sure of Findbugs is complaining about the methods as they are synchronizing on [this] and not really on [this.listener] Please verify.

      Synchronization on field in futile attempt to guard that field
      This method synchronizes on a field in what appears to be an attempt to guard against simultaneous updates to that field. But guarding a field gets a lock on the referenced object, not on the field. This may not provide the mutual exclusion you need, and other threads might be obtaining locks on the referenced objects (for other purposes). An example of this pattern would be:

      public void setSubscriptionExpiredListener(SubscriptionExpiredListener listener)
      {
      if(this.listener != null)
      {
      synchronized(this.listener)

      { this.listener = listener; }

      }
      else
      this.listener = listener;
      }

      public void removeSubscriptionExpiredListener()
      {
      synchronized(this.listener)

      { this.listener = null; }

      }

      [3]org.sakaiproject.calendar.impl.BaseExternalCalendarSubscriptionService
      If listener is null then NPE in synchronized line 1719
      synchronized(listener)
      {
      if (listener != null)

      { listener.subscriptionExpired(key, e); }

      }

      [4] org.sakaiproject.calendar.impl.BaseExternalCalendarSubscriptionService
      Line 1806
      If subscription is null then NPE for subscription.getContext()

      ExternalSubscription s = loadCalendarSubscriptionFromUrl(subscriptionUrl,
      subscription.getContext());
      if (subscription != null)
      userSubscriptions.put(subscriptionUrl, subscription);
      }

      [5] org.sakaiproject.calendar.tool.CalendarAction
      NPE
      Line 1775
      Properties config = placement.getPlacementConfig();
      if (placement != null && config != null)

      [6] org.sakaiproject.calendar.impl.BaseExternalCalendarSubscriptionService
      Makes no sense
      Line 1113
      public void setName(String calendarName)

      { this.m_name = m_name; }

      [7] BaseCalendarService
      Line 1717
      Never used
      Map ids = new HashMap();

      [8] BaseCalendarService
      Line 6009
      Never used
      Options options = new Options(userConfig);

      [9] org.sakaiproject.calendar.tool.CalendarAction
      Line 6665
      Never used
      List events = new Vector();

      [10] org.sakaiproject.calendar.tool.CalendarAction
      Line 3056
      Never used
      CalendarEventVectorObj = new CalendarEventVector();

      [11-27] Repeated many times, please review
      org.sakaiproject.calendar.tool.CalendarAction
      Never used Lines(6477,6467,6454,6532,6553,5331,6898,4285,6491,4474,6512,6879,6910,6071,5209,5196)
      SessionState sstate = ((JetspeedRunData)data).getPortletSessionState(peid);

      [28] org.sakaiproject.tool.summarycalendar.ui.EventSummary
      Line 206
      Return early (perhaps after setHasAttachments) and review beginning of method.
      attachments = new ArrayList(); is never used

      public void setAttachments(List attachments) {
      setHasAttachments(attachments != null && attachments.size() > 0);
      this.attachmentsWrp = new ArrayList();
      if(attachments == null)

      { attachments = new ArrayList(); return; }

      [29] org.sakaiproject.tool.summarycalendar.ui.EventSummary
      Please review what the implications of a malformed/forbidden/invalid attachment are. Is the resource directory getting polluted are system resources leaking.
      Suggest removing try catch block and looking at which exceptions are being thrown and making sure everything is closed and perhaps logged.
      Line 211
      while(it.hasNext()){
      Reference ref = (Reference) it.next();
      try

      { AttachmentWrapper aw = new AttachmentWrapper(); aw.setUrl(ref.getUrl()); aw.setDisplayName(ref.getProperties().getProperty(ResourceProperties.PROP_DISPLAY_NAME)); attachmentsWrp.add(aw); }

      catch(Exception e)

      { // Ignore malformed/forbidden/invalid attachment }

      }

      [30] org.sakaiproject.calendar.tool.CalendarAction
      Verify intention, simplify. Line 3362

      if (vectorObj.isEmpty())

      { events.add(range,vectorObj); }
      else
      { events.add(range,vectorObj); }

      [29] BaseExternalCalendarSubscriptionService
      If you overrid equals then you should also overide hashcode

      [31] org.sakaiproject.calendar.tool.CalendarAction
      Line 7906
      Fail to close InputStream in Exceptional situations.
      this.getClass().getResourceAsStream("calendar.config") needs to explicitly closed on IOException, so you will need to make non anonymous.
      try
      {
      if ( configProps == null )

      { configProps = new Properties(); configProps.load(this.getClass().getResourceAsStream("calendar.config")); }

      }
      catch ( IOException e )

      { M_log.warn("unable to load calendar.config: " + e); }

        Gliffy Diagrams

          Zeplin

            Attachments

              Activity

                People

                Assignee:
                nfernandes Nuno Fernandes (Inactive)
                Reporter:
                a.m.berg@uva.nl Alan Berg
                Votes:
                0 Vote for this issue
                Watchers:
                0 Start watching this issue

                  Dates

                  Created:
                  Updated:
                  Resolved:

                    Git Integration