Improve queue locking and fix linux unit tests (#6)

* Close Backtrace client * Add time limit to await for backtrace client * Add final to variables * Unlock queue after adding message * Decrease waiting time in the unit tests * Add logging * Fix locks * Check fix processing * Add closing lock * Remove debug logging * Improve comment * Reformat code and reorganize imports * Update library version, README.md and CHANGELOG.md * Remove logs
backtrace-labs · Jul 14, 2021 · 7cc0074 · 7cc0074
1 parent 6cb8b12
commit 7cc0074
Show file tree

Hide file tree

Showing 23 changed files with 198 additions and 186 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,8 @@
 # Backtrace Java Release Notes
 
+## Version 0.9.3 - 14.07.2021
+- Improve the queue handling to eliminate rare deadlocks occurred during closing BacktraceClient
+
 ## Version 0.9.2 - 04.05.2021
 - Fix the [issue](https://github.com/backtrace-labs/backtrace-java/issues/4) with excessive CPU usage by the backtrace-thread
 

diff --git a/README.md b/README.md
@@ -17,7 +17,7 @@ We are working closely with users for their initial rollouts of backtrace-java.
 * Gradle
 ```
 dependencies {
-    implementation 'com.github.backtrace-labs.backtrace-java:backtrace-java:0.9.2'
+    implementation 'com.github.backtrace-labs.backtrace-java:backtrace-java:0.9.3'
 }
 ```
 
@@ -26,7 +26,7 @@ dependencies {
 <dependency>
   <groupId>com.github.backtrace-labs.backtrace-java</groupId>
   <artifactId>backtrace-java</artifactId>
-  <version>0.9.2</version>
+  <version>0.9.3</version>
 </dependency>
 ```
 

diff --git a/build.gradle b/build.gradle
@@ -6,7 +6,7 @@ plugins {
 apply from: 'publish.gradle'
 
 group = 'com.github.backtrace.io'
-version = '0.9.2'
+version = '0.9.3'
 
 repositories {
     jcenter()

diff --git a/gradle.properties b/gradle.properties
@@ -1,4 +1,4 @@
-VERSION_NAME=0.9.2
+VERSION_NAME=0.9.3
 
 GROUP=com.github.backtrace-labs.backtrace-java
 

diff --git a/src/main/java/backtrace/io/Backtrace.java b/src/main/java/backtrace/io/Backtrace.java
@@ -32,21 +32,23 @@ class Backtrace {
      * Handles the queue of incoming error reports
      */
     void handleBacktraceMessage() {
+        if (queue.isEmpty()) {
+            this.queue.queueIsEmpty();
+            this.queue.unlock();
+        }
+
         try {
+            if (this.queue.isClosing()) {
+                return;
+            }
             this.queue.awaitNewMessage();
-
             BacktraceMessage message = queue.poll();
-
             if (message != null) {
                 processSingleBacktraceMessage(message);
             }
         } catch (Exception e) {
             LOGGER.error("Exception during pipeline for message from queue..", e);
         }
-
-        if (queue.isEmpty()) {
-            this.queue.unlock();
-        }
     }
 
     /**

diff --git a/src/main/java/backtrace/io/BacktraceQueue.java b/src/main/java/backtrace/io/BacktraceQueue.java
@@ -1,8 +1,6 @@
 package backtrace.io;
 
 import backtrace.io.helpers.CountLatch;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.TimeUnit;
@@ -13,9 +11,9 @@
  */
 class BacktraceQueue extends ConcurrentLinkedQueue<BacktraceMessage> {
 
-    private static final transient Logger LOGGER = LoggerFactory.getLogger(BacktraceQueue.class);
-    private final CountLatch lock = new CountLatch(0, 0);
-    private final CountLatch notEmptyQueue = new CountLatch(1, 0);
+    private final CountLatch processingLock = new CountLatch(0, 0); // state 1 if should process message
+    private final CountLatch notEmptyQueueLock = new CountLatch(1, 0); // state 1 if queue is empty
+    private final CountLatch closingLock = new CountLatch(1, 0); // state 0 if closing
 
     /**
      * Add message to queue with locking semaphore to inform that at least one of messages are processing
@@ -24,41 +22,50 @@ class BacktraceQueue extends ConcurrentLinkedQueue<BacktraceMessage> {
      */
     void addWithLock(BacktraceMessage message) {
         this.lock();
+
         this.add(message);
+
+        this.queueNotEmpty();
     }
 
     /**
      * Unlock semaphore to inform that all messages from queue are sent
      */
     void unlock() {
-        LOGGER.debug("Releasing semaphore..");
-
-        if(notEmptyQueue.getCount() == 0) {
-            notEmptyQueue.countUp();
+        if (processingLock.getCount() == 1) {
+            processingLock.countDown();
         }
+    }
 
-        if (lock.getCount() == 0) {
-            return;
+    boolean shouldHandleMessages() {
+        return processingLock.getCount() == 1;
+    }
+
+    /**
+     * Lock semaphore because queue is empty
+     */
+    void queueIsEmpty() {
+        if (notEmptyQueueLock.getCount() == 0) {
+            notEmptyQueueLock.countUp();
         }
+    }
 
-        lock.countDown();
+    /**
+     * Unlocking processing because queue is not empty
+     */
+    private void queueNotEmpty() {
+        if (notEmptyQueueLock.getCount() == 1) {
+            notEmptyQueueLock.countDown();
+        }
     }
 
     /**
      * Lock semaphore to inform that at least one of messages are processing
      */
     private void lock() {
-        if(notEmptyQueue.getCount() == 1) {
-            notEmptyQueue.countDown();
-        }
-
-        if (lock.getCount() != 0) {
-            return;
+        if (processingLock.getCount() == 0) {
+            processingLock.countUp();
         }
-
-        LOGGER.debug("Locking semaphore..");
-        lock.countUp();
-        LOGGER.debug("Semaphore locked..");
     }
 
     /**
@@ -67,26 +74,27 @@ private void lock() {
      * @throws InterruptedException if the current thread is interrupted while waiting
      */
     void await() throws InterruptedException {
-        LOGGER.debug("Waiting for the semaphore");
-        lock.await();
-        LOGGER.debug("The semaphore has been released");
+        processingLock.await();
+    }
+
+    boolean isClosing() {
+        return closingLock.getCount() == 0;
     }
 
     void close() {
-        if(notEmptyQueue.getCount() == 1) {
-            notEmptyQueue.countDown();
+        if (closingLock.getCount() == 1) {
+            closingLock.countDown();
         }
+        queueNotEmpty();
     }
 
     /**
-     * Wait until all messages in queue will be sent
+     * Wait until new message will be added to the queue
      *
      * @throws InterruptedException if the current thread is interrupted while waiting
      */
     void awaitNewMessage() throws InterruptedException {
-        LOGGER.debug("Waiting until queue will not be empty");
-        notEmptyQueue.await();
-        LOGGER.debug("Queue is not empty");
+        notEmptyQueueLock.await();
     }
 
 
@@ -101,9 +109,7 @@ void awaitNewMessage() throws InterruptedException {
      */
     boolean await(long timeout,
                   TimeUnit unit) throws InterruptedException {
-        LOGGER.debug("Waiting for the semaphore");
-        boolean result = lock.await(timeout, unit);
-        LOGGER.debug("The semaphore has been released");
+        boolean result = processingLock.await(timeout, unit);
         return result;
     }
 }
diff --git a/src/main/java/backtrace/io/BacktraceQueueHandler.java b/src/main/java/backtrace/io/BacktraceQueueHandler.java
@@ -40,7 +40,7 @@ void send(BacktraceReport report, Map<String, Object> attributes, OnServerRespon
      * @throws InterruptedException if the current thread is interrupted while waiting
      */
     void close() throws InterruptedException {
-        if(!this.thread.isAlive()){
+        if (!this.thread.isAlive()) {
             return;
         }
         this.thread.close();

diff --git a/src/main/java/backtrace/io/BacktraceThread.java b/src/main/java/backtrace/io/BacktraceThread.java
@@ -9,9 +9,9 @@
 public class BacktraceThread extends Thread {
     private static final transient Logger LOGGER = LoggerFactory.getLogger(BacktraceThread.class);
     private final static String THREAD_NAME = "backtrace-daemon";
-    private Backtrace backtrace;
+    private final Backtrace backtrace;
     private volatile boolean running = true;
-    private CountDownLatch closing = new CountDownLatch(1);
+    private final CountDownLatch closing = new CountDownLatch(1);
 
     /**
      * Creates new thread for handling and sending error reports passed to queue

diff --git a/src/main/java/backtrace/io/data/Attributes.java b/src/main/java/backtrace/io/data/Attributes.java
@@ -17,9 +17,9 @@
  */
 class Attributes {
     private static final transient Logger LOGGER = LoggerFactory.getLogger(backtrace.io.data.Attributes.class);
-    
+
     private static final String MACHINE_ID = generateMachineId();
-    
+
     /**
      * Gets built-in primitive attributes
      */

diff --git a/src/main/java/backtrace/io/data/BacktraceData.java b/src/main/java/backtrace/io/data/BacktraceData.java
@@ -3,7 +3,6 @@
 import backtrace.io.data.report.SourceCode;
 import backtrace.io.data.report.SourceCodeData;
 import backtrace.io.data.report.ThreadData;
-
 import backtrace.io.data.report.ThreadInformation;
 import backtrace.io.helpers.FileHelper;
 import com.google.gson.annotations.SerializedName;
@@ -127,7 +126,7 @@ public BacktraceData(BacktraceReport report) {
      * @param clientAttributes Attributes which should be added to BacktraceData object
      */
     public BacktraceData(BacktraceReport report, Map<String, Object> clientAttributes) {
-        this(report,clientAttributes, true);
+        this(report, clientAttributes, true);
     }
 
     /**
@@ -203,6 +202,7 @@ private String getLibraryVersion() {
 
     /**
      * Sets information about all threads
+     *
      * @param allThreads if true information about all threads will be gathered
      */
     private void setThreadsInformation(boolean allThreads) {

diff --git a/src/main/java/backtrace/io/data/BacktraceReport.java b/src/main/java/backtrace/io/data/BacktraceReport.java
@@ -180,14 +180,14 @@ public BacktraceReport(
      * Creates new instance of Backtrace report to sending a report
      * with user message, application exception and attributes
      *
-     * @param message         Custom client message
-     * @param exception       Current exception
-     * @param attributes      Additional information about application state
+     * @param message    Custom client message
+     * @param exception  Current exception
+     * @param attributes Additional information about application state
      */
     public BacktraceReport(
             String message,
             Exception exception,
-            Map<String, Object> attributes){
+            Map<String, Object> attributes) {
         this(message, exception, attributes, null);
     }
 
@@ -202,20 +202,20 @@ public BacktraceReport(
     public BacktraceReport(
             String message,
             Exception exception,
-            List<String> attachmentPaths){
+            List<String> attachmentPaths) {
         this(message, exception, null, attachmentPaths);
     }
 
     /**
      * Creates new instance of Backtrace report to sending a report
      * with message and application exception
      *
-     * @param message         Custom client message
-     * @param exception       Current exception
+     * @param message   Custom client message
+     * @param exception Current exception
      */
     public BacktraceReport(
             String message,
-            Exception exception){
+            Exception exception) {
         this(message, exception, null, null);
     }
 

diff --git a/src/main/java/backtrace/io/data/report/ThreadData.java b/src/main/java/backtrace/io/data/report/ThreadData.java
@@ -40,11 +40,11 @@ public Map<String, ThreadInformation> getThreadInformation() {
      * Create instance of ThreadData class to collect information about used threads
      *
      * @param exceptionStack current BacktraceReport exception stack
-     * @param allThreads if true information about all threads will be gathered
+     * @param allThreads     if true information about all threads will be gathered
      */
     public ThreadData(ArrayList<BacktraceStackFrame> exceptionStack, boolean allThreads) {
         generateCurrentThreadInformation(exceptionStack);
-        if(allThreads) {
+        if (allThreads) {
             processThreads();
         }
     }

diff --git a/src/main/java/backtrace/io/http/ApiSender.java b/src/main/java/backtrace/io/http/ApiSender.java
@@ -20,7 +20,7 @@ public class ApiSender {
     /**
      * Send HTTP request for certain url server with information about device, error, attachments
      *
-     * @param serverUrl server http address to which the request will be sent
+     * @param serverUrl     server http address to which the request will be sent
      * @param backtraceData error report
      * @return information from the server about the result of processing the request
      */

diff --git a/src/main/java/backtrace/io/http/BacktraceResult.java b/src/main/java/backtrace/io/http/BacktraceResult.java
@@ -109,8 +109,8 @@ public static BacktraceResult onError(BacktraceReport report, Exception exceptio
     /**
      * Returns result when error occurs while sending data to API
      *
-     * @param report    executed report
-     * @param exception current exception
+     * @param report         executed report
+     * @param exception      current exception
      * @param httpStatusCode returned http status code
      * @return BacktraceResult with exception information
      */

diff --git a/src/test/java/backtrace/io/BacktraceClientTest.java b/src/test/java/backtrace/io/BacktraceClientTest.java
@@ -33,7 +33,6 @@ public void closeBacktraceClient() throws InterruptedException {
 
         boolean isBacktraceThreadRunningAfterClose = isBacktraceThreadRunning();
 
-        System.out.println(isBacktraceThreadRunning);
         // THEN
         Assert.assertTrue(isBacktraceThreadRunning);
         Assert.assertFalse(isBacktraceThreadRunningAfterClose);
@@ -62,19 +61,20 @@ public BacktraceResult onRequest(BacktraceData data) {
         boolean isBacktraceThreadRunning = isBacktraceThreadRunning();
         backtraceClient.send("test-message");
         backtraceClient.close();
+
         boolean isBacktraceThreadRunningAfterClose = isBacktraceThreadRunning();
-        waiter.await();
+        waiter.await(5, TimeUnit.SECONDS);
 
         // THEN
         Assert.assertTrue(isBacktraceThreadRunning);
         Assert.assertFalse(isBacktraceThreadRunningAfterClose);
     }
 
-    private boolean isBacktraceThreadRunning(){
+    private boolean isBacktraceThreadRunning() {
         Set<Thread> threads = Thread.getAllStackTraces().keySet();
 
         for (Thread t : threads) {
-            if (t == null){
+            if (t == null) {
                 continue;
             }
             if (t.getName().equals(THREAD_NAME)) {