Merge pull request #126 from testmycode/fix-125

Fix #125 (potential tight loop in sending events)

Author: jamo

tmc-plugin/src/fi/helsinki/cs/tmc/spyware/EventRateLimiter.java

@@ -1,5 +1,7 @@
 package fi.helsinki.cs.tmc.spyware;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import com.google.common.collect.Iterables;
 import fi.helsinki.cs.tmc.data.Course;
 import fi.helsinki.cs.tmc.model.CourseDb;
@@ -33,6 +35,7 @@ public class EventSendBuffer implements EventReceiver {
     public static final int DEFAULT_MAX_EVENTS = 64 * 1024;
     public static final int DEFAULT_AUTOSEND_THREHSOLD = DEFAULT_MAX_EVENTS / 2;
     public static final int DEFAULT_AUTOSEND_COOLDOWN = 30*1000;
+    public static final int DEFAULT_MAX_EVENTS_PER_SEND = 500;
 
     private Random random = new Random();
     private SpywareSettings settings;
@@ -45,6 +48,7 @@ public class EventSendBuffer implements EventReceiver {
     private int eventsToRemoveAfterSend = 0;
     private int maxEvents = DEFAULT_MAX_EVENTS;
     private int autosendThreshold = DEFAULT_AUTOSEND_THREHSOLD;
+    private int maxEventsPerSend = DEFAULT_MAX_EVENTS_PER_SEND;  // Servers have POST size limits
     private Cooldown autosendCooldown;
 
 
@@ -70,17 +74,17 @@ public EventSendBuffer(SpywareSettings settings, ServerAccess serverAccess, Cour
     }
 
     public void setSendingInterval(long interval) {
+        checkArgument(interval >= 0);
         this.sendingTask.setInterval(interval);
     }
 
     public void setSavingInterval(long interval) {
+        checkArgument(interval >= 0);
         this.savingTask.setInterval(interval);
     }
 
     public void setMaxEvents(int newMaxEvents) {
-        if (newMaxEvents <= 0) {
-            throw new IllegalArgumentException();
-        }
+        checkArgument(newMaxEvents > 0);
 
         synchronized (sendQueue) {
             if (newMaxEvents < maxEvents) {
@@ -107,7 +111,17 @@ public void setAutosendThreshold(int autosendThreshold) {
     }
 
     public void setAutosendCooldown(long durationMillis) {
-        this.autosendCooldown.setDurationMillis(durationMillis);
+        checkArgument(durationMillis > 0);
+        synchronized (sendQueue) {
+            this.autosendCooldown.setDurationMillis(durationMillis);
+        }
+    }
+
+    public void setMaxEventsPerSend(int maxEventsPerSend) {
+        checkArgument(maxEventsPerSend > 0);
+        synchronized (sendQueue) {
+            this.maxEventsPerSend = maxEventsPerSend;
+        }
     }
 
     public void sendNow() {
@@ -174,9 +188,6 @@ public void close() {
 
 
     private SingletonTask sendingTask = new SingletonTask(new Runnable() {
-        // Sending too many at once may go over the server's POST size limit.
-        private static final int MAX_EVENTS_PER_SEND = 500;
-
         @Override
         public void run() {
             boolean shouldSendMore;
@@ -198,7 +209,9 @@ public void run() {
 
                 log.log(Level.INFO, "Sending {0} events to {1}", new Object[] { eventsToSend.size(), url });
 
-                doSend(eventsToSend, url);
+                if (!tryToSend(eventsToSend, url)) {
+                    shouldSendMore = false;
+                }
             } while (shouldSendMore);
         }
 
@@ -207,7 +220,7 @@ private ArrayList<LoggableEvent> copyEventsToSendFromQueue() {
                 ArrayList<LoggableEvent> eventsToSend = new ArrayList<LoggableEvent>(sendQueue.size());
 
                 Iterator<LoggableEvent> i = sendQueue.iterator();
-                while (i.hasNext() && eventsToSend.size() < MAX_EVENTS_PER_SEND) {
+                while (i.hasNext() && eventsToSend.size() < maxEventsPerSend) {
                     eventsToSend.add(i.next());
                 }
 
@@ -235,7 +248,7 @@ private String pickDestinationUrl() {
             return url;
         }
 
-        private void doSend(final ArrayList<LoggableEvent> eventsToSend, final String url) {
+        private boolean tryToSend(final ArrayList<LoggableEvent> eventsToSend, final String url) {
             CancellableCallable<Object> task = serverAccess.getSendEventLogJob(url, eventsToSend);
             Future<Object> future = BgTask.start("Sending stats", task);
 
@@ -245,7 +258,7 @@ private void doSend(final ArrayList<LoggableEvent> eventsToSend, final String ur
                 future.cancel(true);
             } catch (ExecutionException ex) {
                 log.log(Level.INFO, "Sending failed", ex);
-                return;
+                return false;
             }
 
             log.log(Level.INFO, "Sent {0} events successfully to {1}", new Object[] { eventsToSend.size(), url });
@@ -256,6 +269,7 @@ private void doSend(final ArrayList<LoggableEvent> eventsToSend, final String ur
             // then we may end up sending duplicate events later.
             // This will hopefully be very rare.
             savingTask.start();
+            return true;
         }
 
         private void removeSentEventsFromQueue() {

tmc-plugin/src/fi/helsinki/cs/tmc/spyware/EventSendBuffer.java

@@ -6,19 +6,20 @@
 import fi.helsinki.cs.tmc.data.CourseListUtils;
 import fi.helsinki.cs.tmc.data.Exercise;
 import fi.helsinki.cs.tmc.data.ExerciseListUtils;
+import java.util.Date;
 import java.util.GregorianCalendar;
 import java.util.List;
 import org.junit.Before;
 
 public class CourseListParserTest {
-    
+
     private CourseListParser parser;
-    
+
     @Before
     public void setUp() {
         parser = new CourseListParser();
     }
-    
+
     @Test
     public void itShouldParseJsonCourseLists() {
         String exercisesJson =
@@ -33,30 +34,33 @@ public void itShouldParseJsonCourseLists() {
                 "checksum: \"123abc\"" +
                 "}]";
         String json = "{api_version: 1, courses: [{\"name\": \"TheCourse\",\"exercises\": " + exercisesJson + "}]}";
-        
+
         List<Course> result = parser.parseFromJson(json);
-        
+
         Course course = CourseListUtils.getCourseByName(result, "TheCourse");
         assertEquals("TheCourse", course.getName());
-        
+
         Exercise exercise = ExerciseListUtils.getExerciseByName(course.getExercises(), "TheExercise");
-        
+
         assertEquals("TheCourse", exercise.getCourseName());
-        
+
         GregorianCalendar cal = new GregorianCalendar();
         cal.setTime(exercise.getDeadline());
         assertEquals(2015, cal.get(GregorianCalendar.YEAR));
         assertEquals(1, cal.get(GregorianCalendar.HOUR_OF_DAY));
         assertEquals(30, cal.get(GregorianCalendar.MINUTE));
-        
+
         assertEquals("http://example.com/courses/123/exercises/1.zip", exercise.getDownloadUrl());
         assertEquals("http://example.com/courses/123/exercises/1/submissions", exercise.getReturnUrl());
         assertTrue(exercise.isAttempted());
         assertFalse(exercise.isCompleted());
+
+        exercise.setDeadline(new Date(new Date().getTime() + 60 * 60 * 1000));
         assertTrue(exercise.isReturnable());
+
         assertEquals("123abc", exercise.getChecksum());
     }
-    
+
     @Test
     public void itShouldParseAnEmptyJsonArrayAsAnEmptyCourseList() {
         List<Course> empty = parser.parseFromJson("{api_version: 1, courses: []}");
@@ -67,12 +71,12 @@ public void itShouldParseAnEmptyJsonArrayAsAnEmptyCourseList() {
     public void itShouldThrowAnNullPointerExceptionIfTheInputIsEmpty() throws Exception {
         parser.parseFromJson(null);
     }
-    
+
     @Test(expected = IllegalArgumentException.class)
     public void itShouldThrowAnIllegalArgumentExceptionIfTheInputIsEmpty() throws Exception {
         parser.parseFromJson("   ");
     }
-    
+
     @Test
     public void itShouldParseANullDeadlineAsNull() {
         String exercisesJson =
@@ -86,9 +90,9 @@ public void itShouldParseANullDeadlineAsNull() {
                 "returnable: true" +
                 "}]";
         String json = "{api_version: 1, courses: [{\"name\": \"TheCourse\",\"exercises\": " + exercisesJson + "}]}";
-        
+
         List<Course> result = parser.parseFromJson(json);
-        
+
         assertNull(result.get(0).getExercises().get(0).getDeadline());
     }
 }