Skip to content

Commit 471145f

Browse files
mpartelmpartel
committed
Fixed a tight retry loop in EventSendBuffer.
This would get triggered e.g. by authentication failure. Fixes #125.
1 parent 1d8f79a commit 471145f

File tree

2 files changed

+54
-25
lines changed

2 files changed

+54
-25
lines changed

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

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package fi.helsinki.cs.tmc.spyware;
22

3+
import static com.google.common.base.Preconditions.checkArgument;
4+
35
import com.google.common.collect.Iterables;
46
import fi.helsinki.cs.tmc.data.Course;
57
import fi.helsinki.cs.tmc.model.CourseDb;
@@ -33,6 +35,7 @@ public class EventSendBuffer implements EventReceiver {
3335
public static final int DEFAULT_MAX_EVENTS = 64 * 1024;
3436
public static final int DEFAULT_AUTOSEND_THREHSOLD = DEFAULT_MAX_EVENTS / 2;
3537
public static final int DEFAULT_AUTOSEND_COOLDOWN = 30*1000;
38+
public static final int DEFAULT_MAX_EVENTS_PER_SEND = 500;
3639

3740
private Random random = new Random();
3841
private SpywareSettings settings;
@@ -45,6 +48,7 @@ public class EventSendBuffer implements EventReceiver {
4548
private int eventsToRemoveAfterSend = 0;
4649
private int maxEvents = DEFAULT_MAX_EVENTS;
4750
private int autosendThreshold = DEFAULT_AUTOSEND_THREHSOLD;
51+
private int maxEventsPerSend = DEFAULT_MAX_EVENTS_PER_SEND; // Servers have POST size limits
4852
private Cooldown autosendCooldown;
4953

5054

@@ -70,17 +74,17 @@ public EventSendBuffer(SpywareSettings settings, ServerAccess serverAccess, Cour
7074
}
7175

7276
public void setSendingInterval(long interval) {
77+
checkArgument(interval >= 0);
7378
this.sendingTask.setInterval(interval);
7479
}
7580

7681
public void setSavingInterval(long interval) {
82+
checkArgument(interval >= 0);
7783
this.savingTask.setInterval(interval);
7884
}
7985

8086
public void setMaxEvents(int newMaxEvents) {
81-
if (newMaxEvents <= 0) {
82-
throw new IllegalArgumentException();
83-
}
87+
checkArgument(newMaxEvents > 0);
8488

8589
synchronized (sendQueue) {
8690
if (newMaxEvents < maxEvents) {
@@ -107,7 +111,17 @@ public void setAutosendThreshold(int autosendThreshold) {
107111
}
108112

109113
public void setAutosendCooldown(long durationMillis) {
110-
this.autosendCooldown.setDurationMillis(durationMillis);
114+
checkArgument(durationMillis > 0);
115+
synchronized (sendQueue) {
116+
this.autosendCooldown.setDurationMillis(durationMillis);
117+
}
118+
}
119+
120+
public void setMaxEventsPerSend(int maxEventsPerSend) {
121+
checkArgument(maxEventsPerSend > 0);
122+
synchronized (sendQueue) {
123+
this.maxEventsPerSend = maxEventsPerSend;
124+
}
111125
}
112126

113127
public void sendNow() {
@@ -174,9 +188,6 @@ public void close() {
174188

175189

176190
private SingletonTask sendingTask = new SingletonTask(new Runnable() {
177-
// Sending too many at once may go over the server's POST size limit.
178-
private static final int MAX_EVENTS_PER_SEND = 500;
179-
180191
@Override
181192
public void run() {
182193
boolean shouldSendMore;
@@ -198,7 +209,9 @@ public void run() {
198209

199210
log.log(Level.INFO, "Sending {0} events to {1}", new Object[] { eventsToSend.size(), url });
200211

201-
doSend(eventsToSend, url);
212+
if (!tryToSend(eventsToSend, url)) {
213+
shouldSendMore = false;
214+
}
202215
} while (shouldSendMore);
203216
}
204217

@@ -207,7 +220,7 @@ private ArrayList<LoggableEvent> copyEventsToSendFromQueue() {
207220
ArrayList<LoggableEvent> eventsToSend = new ArrayList<LoggableEvent>(sendQueue.size());
208221

209222
Iterator<LoggableEvent> i = sendQueue.iterator();
210-
while (i.hasNext() && eventsToSend.size() < MAX_EVENTS_PER_SEND) {
223+
while (i.hasNext() && eventsToSend.size() < maxEventsPerSend) {
211224
eventsToSend.add(i.next());
212225
}
213226

@@ -235,7 +248,7 @@ private String pickDestinationUrl() {
235248
return url;
236249
}
237250

238-
private void doSend(final ArrayList<LoggableEvent> eventsToSend, final String url) {
251+
private boolean tryToSend(final ArrayList<LoggableEvent> eventsToSend, final String url) {
239252
CancellableCallable<Object> task = serverAccess.getSendEventLogJob(url, eventsToSend);
240253
Future<Object> future = BgTask.start("Sending stats", task);
241254

@@ -245,7 +258,7 @@ private void doSend(final ArrayList<LoggableEvent> eventsToSend, final String ur
245258
future.cancel(true);
246259
} catch (ExecutionException ex) {
247260
log.log(Level.INFO, "Sending failed", ex);
248-
return;
261+
return false;
249262
}
250263

251264
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
256269
// then we may end up sending duplicate events later.
257270
// This will hopefully be very rare.
258271
savingTask.start();
272+
return true;
259273
}
260274

261275
private void removeSentEventsFromQueue() {

‎tmc-plugin/test/unit/src/fi/helsinki/cs/tmc/spyware/EventSendBufferTest.java‎

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import java.util.Set;
1212
import java.util.concurrent.Semaphore;
1313
import java.util.concurrent.TimeoutException;
14+
import java.util.concurrent.atomic.AtomicInteger;
1415
import org.junit.After;
1516
import org.junit.Before;
1617
import org.junit.Test;
@@ -42,8 +43,8 @@ public class EventSendBufferTest {
4243
private long sendDuration;
4344
private Semaphore sendStartSemaphore; // released when a send starts
4445
private Exception sendException;
45-
private volatile int sendOperationsStarted;
46-
private volatile int sendOperationsFinished;
46+
private AtomicInteger sendOperationsStarted;
47+
private AtomicInteger sendOperationsFinished;
4748

4849
@Captor
4950
private ArgumentCaptor<LoggableEvent[]> savedEvents;
@@ -64,18 +65,18 @@ public void setUp() throws IOException {
6465
sendDuration = 0;
6566
sendStartSemaphore = new Semaphore(0);
6667
sendException = null;
67-
sendOperationsStarted = 0;
68-
sendOperationsFinished = 0;
68+
sendOperationsStarted = new AtomicInteger(0);
69+
sendOperationsFinished = new AtomicInteger(0);
6970
when(serverAccess.getSendEventLogJob(spywareServerUrl.capture(), sentEvents.capture())).thenReturn(new CancellableCallable<Object>() {
7071
@Override
7172
public Object call() throws Exception {
72-
sendOperationsStarted++;
73+
sendOperationsStarted.incrementAndGet();
7374
sendStartSemaphore.release();
7475
Thread.sleep(sendDuration);
7576
if (sendException != null) {
7677
throw sendException;
7778
}
78-
sendOperationsFinished++;
79+
sendOperationsFinished.incrementAndGet();
7980
return null;
8081
}
8182

@@ -165,7 +166,7 @@ public void autosendsPeriodically() throws InterruptedException {
165166
sender.receiveEvent(ev2);
166167
Thread.sleep(250);
167168

168-
assertTrue(sendOperationsFinished >= 2);
169+
assertTrue(sendOperationsFinished.get() >= 2);
169170
}
170171

171172
@Test
@@ -192,12 +193,12 @@ public void autosendsWhenNumberOfEventsGoesOverThreshold() throws TimeoutExcepti
192193
sender.receiveEvent(ev1);
193194
sender.receiveEvent(ev2);
194195
Thread.sleep(50);
195-
assertEquals(0, sendOperationsFinished);
196+
assertEquals(0, sendOperationsFinished.get());
196197

197198
sender.receiveEvent(ev3);
198199
sender.waitUntilCurrentSendingFinished(1000);
199200

200-
assertEquals(1, sendOperationsFinished);
201+
assertEquals(1, sendOperationsFinished.get());
201202
LoggableEvent[] expecteds = new LoggableEvent[] { ev1, ev2, ev3 };
202203
assertArrayEquals(expecteds, sentEvents.getValue().toArray(new LoggableEvent[0]));
203204
}
@@ -214,12 +215,26 @@ public void autosendingWhenOverThresholdHasACooldown() throws TimeoutException,
214215
sender.receiveEvent(ev4);
215216
sender.waitUntilCurrentSendingFinished(1000);
216217

217-
assertEquals(1, sendOperationsStarted);
218+
assertEquals(1, sendOperationsStarted.get());
218219
LoggableEvent[] expecteds = new LoggableEvent[] { ev1, ev2, ev3 };
219220
assertArrayEquals(expecteds, sentEvents.getValue().toArray(new LoggableEvent[0]));
220221
}
221222

222-
@Test
223+
@Test // Issue #125
224+
public void retryLoopRespectAutosendIntervalOnFailureEvenIfThereIsMoreToSend() throws TimeoutException, InterruptedException {
225+
sendException = new RuntimeException("Sending failed");
226+
sender.setMaxEventsPerSend(2);
227+
sender.receiveEvent(ev1);
228+
sender.receiveEvent(ev2);
229+
sender.receiveEvent(ev3);
230+
sender.sendNow();
231+
sender.waitUntilCurrentSendingFinished(1000);
232+
233+
assertEquals(1, sendOperationsStarted.get());
234+
assertEquals(0, sendOperationsFinished.get());
235+
}
236+
237+
@Test // FIXME: this test appears to be flaky
223238
public void discardsOldestEventsOnOverflow() throws TimeoutException, InterruptedException {
224239
sender.setMaxEvents(3);
225240

@@ -251,7 +266,7 @@ public void sendsEventsReceivedDuringSendingInSubsequentSend() throws TimeoutExc
251266
sender.sendNow();
252267
sender.waitUntilCurrentSendingFinished(1000);
253268

254-
assertEquals(2, sendOperationsFinished);
269+
assertEquals(2, sendOperationsFinished.get());
255270
assertEquals(2, sentEvents.getAllValues().size());
256271

257272
assertArrayEquals(new LoggableEvent[] { ev1 }, sentEvents.getAllValues().get(0).toArray(new LoggableEvent[0]));
@@ -286,7 +301,7 @@ public void toleratesOverflowDuringSending() throws TimeoutException, Interrupte
286301
sender.sendNow();
287302
sender.waitUntilCurrentSendingFinished(1000);
288303

289-
assertEquals(3, sendOperationsFinished);
304+
assertEquals(3, sendOperationsFinished.get());
290305
assertEquals(3, sentEvents.getAllValues().size());
291306

292307
assertArrayEquals(new LoggableEvent[] { ev1 }, sentEvents.getAllValues().get(0).toArray(new LoggableEvent[0]));
@@ -320,7 +335,7 @@ public void retainsEventsForNextSendIfSendingFails() throws TimeoutException, In
320335
sender.waitUntilCurrentSendingFinished(1000);
321336

322337
assertEquals(2, sendStartSemaphore.availablePermits());
323-
assertEquals(1, sendOperationsFinished);
338+
assertEquals(1, sendOperationsFinished.get());
324339
assertArrayEquals(new LoggableEvent[] { ev1 }, sentEvents.getValue().toArray(new LoggableEvent[0]));
325340

326341
Thread.sleep(100); // Wait for save

0 commit comments

Comments
 (0)