Commit b0749784 authored by Abraham Kiggundu's avatar Abraham Kiggundu

Improved encapsulation of thread synchronisation as follows

- replaced use of Object instance mutex with a private final Lock object
- replaced Object signaling with specific condition signalling
parent 276dcb10
build
.gradle
.metadata
*.tmp
......@@ -11,6 +11,8 @@ import static java.util.logging.Level.WARNING;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.Executor;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Logger;
import javax.inject.Inject;
......@@ -64,6 +66,8 @@ Service, EventListener {
private int nextRequestId = 0; // Locking: this
private volatile Settings settings = new Settings();
private final Lock synchLock = new ReentrantLock();
@Inject
public AndroidNotificationManagerImpl(DatabaseComponent db,
......@@ -103,19 +107,31 @@ Service, EventListener {
if(e instanceof SettingsUpdatedEvent) loadSettings();
}
public synchronized void showPrivateMessageNotification(ContactId c) {
Integer count = contactCounts.get(c);
if(count == null) contactCounts.put(c, 1);
else contactCounts.put(c, count + 1);
privateTotal++;
updatePrivateMessageNotification();
public void showPrivateMessageNotification(ContactId c) {
synchLock.lock();
try{
Integer count = contactCounts.get(c);
if(count == null) contactCounts.put(c, 1);
else contactCounts.put(c, count + 1);
privateTotal++;
updatePrivateMessageNotification();
}
finally{
synchLock.unlock();
}
}
public synchronized void clearPrivateMessageNotification(ContactId c) {
Integer count = contactCounts.remove(c);
if(count == null) return; // Already cleared
privateTotal -= count;
updatePrivateMessageNotification();
public void clearPrivateMessageNotification(ContactId c) {
synchLock.lock();
try{
Integer count = contactCounts.remove(c);
if(count == null) return; // Already cleared
privateTotal -= count;
updatePrivateMessageNotification();
}
finally{
synchLock.unlock();
}
}
// Locking: this
......@@ -180,19 +196,31 @@ Service, EventListener {
return defaults;
}
public synchronized void showGroupPostNotification(GroupId g) {
Integer count = groupCounts.get(g);
if(count == null) groupCounts.put(g, 1);
else groupCounts.put(g, count + 1);
groupTotal++;
updateGroupPostNotification();
public void showGroupPostNotification(GroupId g) {
synchLock.lock();
try{
Integer count = groupCounts.get(g);
if(count == null) groupCounts.put(g, 1);
else groupCounts.put(g, count + 1);
groupTotal++;
updateGroupPostNotification();
}
finally{
synchLock.unlock();
}
}
public synchronized void clearGroupPostNotification(GroupId g) {
public void clearGroupPostNotification(GroupId g) {
synchLock.lock();
try{
Integer count = groupCounts.remove(g);
if(count == null) return; // Already cleared
groupTotal -= count;
updateGroupPostNotification();
}
finally{
synchLock.unlock();
}
}
// Locking: this
......@@ -238,18 +266,23 @@ Service, EventListener {
}
}
// Locking: this
private void clearGroupPostNotification() {
Object o = appContext.getSystemService(NOTIFICATION_SERVICE);
NotificationManager nm = (NotificationManager) o;
nm.cancel(GROUP_POST_NOTIFICATION_ID);
}
public synchronized void clearNotifications() {
contactCounts.clear();
groupCounts.clear();
privateTotal = groupTotal = 0;
clearPrivateMessageNotification();
clearGroupPostNotification();
public void clearNotifications() {
synchLock.lock();
try{
contactCounts.clear();
groupCounts.clear();
privateTotal = groupTotal = 0;
clearPrivateMessageNotification();
clearGroupPostNotification();
}
finally{
synchLock.unlock();
}
}
}
......@@ -4,6 +4,8 @@ import static java.util.logging.Level.INFO;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Logger;
import org.briarproject.api.android.ReferenceManager;
......@@ -13,49 +15,70 @@ class ReferenceManagerImpl implements ReferenceManager {
private static final Logger LOG =
Logger.getLogger(ReferenceManagerImpl.class.getName());
// Locking: this
private final Map<Class<?>, Map<Long, Object>> outerMap =
new HashMap<Class<?>, Map<Long, Object>>();
private long nextHandle = 0; // Locking: this
public synchronized <T> T getReference(long handle, Class<T> c) {
Map<Long, Object> innerMap = outerMap.get(c);
if(innerMap == null) {
private final Lock synchLock = new ReentrantLock();
public <T> T getReference(long handle, Class<T> c) {
synchLock.lock();
try{
Map<Long, Object> innerMap = outerMap.get(c);
if(innerMap == null) {
if(LOG.isLoggable(INFO))
LOG.info("0 handles for " + c.getName());
return null;
}
if(LOG.isLoggable(INFO))
LOG.info("0 handles for " + c.getName());
return null;
LOG.info(innerMap.size() + " handles for " + c.getName());
Object o = innerMap.get(handle);
return c.cast(o);
}
finally{
synchLock.unlock();
}
if(LOG.isLoggable(INFO))
LOG.info(innerMap.size() + " handles for " + c.getName());
Object o = innerMap.get(handle);
return c.cast(o);
}
public synchronized <T> long putReference(T reference, Class<T> c) {
Map<Long, Object> innerMap = outerMap.get(c);
if(innerMap == null) {
innerMap = new HashMap<Long, Object>();
outerMap.put(c, innerMap);
public <T> long putReference(T reference, Class<T> c) {
synchLock.lock();
try{
Map<Long, Object> innerMap = outerMap.get(c);
if(innerMap == null) {
innerMap = new HashMap<Long, Object>();
outerMap.put(c, innerMap);
}
long handle = nextHandle++;
innerMap.put(handle, reference);
if(LOG.isLoggable(INFO)) {
LOG.info(innerMap.size() + " handles for " + c.getName() +
" after put");
}
return handle;
}
long handle = nextHandle++;
innerMap.put(handle, reference);
if(LOG.isLoggable(INFO)) {
LOG.info(innerMap.size() + " handles for " + c.getName() +
" after put");
finally{
synchLock.unlock();
}
return handle;
}
public synchronized <T> T removeReference(long handle, Class<T> c) {
Map<Long, Object> innerMap = outerMap.get(c);
if(innerMap == null) return null;
Object o = innerMap.remove(handle);
if(innerMap.isEmpty()) outerMap.remove(c);
if(LOG.isLoggable(INFO)) {
LOG.info(innerMap.size() + " handles for " + c.getName() +
" after remove");
public <T> T removeReference(long handle, Class<T> c) {
synchLock.lock();
try{
Map<Long, Object> innerMap = outerMap.get(c);
if(innerMap == null) return null;
Object o = innerMap.remove(handle);
if(innerMap.isEmpty()) outerMap.remove(c);
if(LOG.isLoggable(INFO)) {
LOG.info(innerMap.size() + " handles for " + c.getName() +
" after remove");
}
return c.cast(o);
}
return c.cast(o);
finally{
synchLock.unlock();
}
}
}
package org.briarproject.crypto;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import org.briarproject.api.crypto.MessageDigest;
import org.spongycastle.crypto.BlockCipher;
import org.spongycastle.crypto.digests.SHA256Digest;
......@@ -23,61 +26,89 @@ class FortunaGenerator {
private final byte[] counter = new byte[BLOCK_BYTES];
private final byte[] buffer = new byte[BLOCK_BYTES];
private final byte[] newKey = new byte[KEY_BYTES];
private final Lock synchLock = new ReentrantLock();
FortunaGenerator(byte[] seed) {
reseed(seed);
}
synchronized void reseed(byte[] seed) {
digest.update(key);
digest.update(seed);
digest.digest(key, 0, KEY_BYTES);
incrementCounter();
void reseed(byte[] seed) {
synchLock.lock();
try{
digest.update(key);
digest.update(seed);
digest.digest(key, 0, KEY_BYTES);
incrementCounter();
}
finally{
synchLock.unlock();
}
}
// Package access for testing
synchronized void incrementCounter() {
counter[0]++;
for(int i = 0; counter[i] == 0; i++) {
if(i + 1 == BLOCK_BYTES)
throw new RuntimeException("Counter exhausted");
counter[i + 1]++;
void incrementCounter() {
synchLock.lock();
try{
counter[0]++;
for(int i = 0; counter[i] == 0; i++) {
if(i + 1 == BLOCK_BYTES)
throw new RuntimeException("Counter exhausted");
counter[i + 1]++;
}
}
}
finally{
synchLock.unlock();
}
}
// Package access for testing
synchronized byte[] getCounter() {
return counter;
byte[] getCounter() {
synchLock.lock();
try{
return counter;
}
finally{
synchLock.unlock();
}
}
synchronized int nextBytes(byte[] dest, int off, int len) {
// Don't write more than the maximum number of bytes in one request
if(len > MAX_BYTES_PER_REQUEST) len = MAX_BYTES_PER_REQUEST;
cipher.init(true, new KeyParameter(key));
// Generate full blocks directly into the output buffer
int fullBlocks = len / BLOCK_BYTES;
for(int i = 0; i < fullBlocks; i++) {
cipher.processBlock(counter, 0, dest, off + i * BLOCK_BYTES);
incrementCounter();
int nextBytes(byte[] dest, int off, int len) {
synchLock.lock();
try{
// Don't write more than the maximum number of bytes in one request
if(len > MAX_BYTES_PER_REQUEST) len = MAX_BYTES_PER_REQUEST;
cipher.init(true, new KeyParameter(key));
// Generate full blocks directly into the output buffer
int fullBlocks = len / BLOCK_BYTES;
for(int i = 0; i < fullBlocks; i++) {
cipher.processBlock(counter, 0, dest, off + i * BLOCK_BYTES);
incrementCounter();
}
// Generate a partial block if needed
int done = fullBlocks * BLOCK_BYTES, remaining = len - done;
assert remaining < BLOCK_BYTES;
if(remaining > 0) {
cipher.processBlock(counter, 0, buffer, 0);
incrementCounter();
// Copy the partial block to the output buffer and erase our copy
System.arraycopy(buffer, 0, dest, off + done, remaining);
for(int i = 0; i < BLOCK_BYTES; i++) buffer[i] = 0;
}
// Generate a new key
for(int i = 0; i < KEY_BYTES / BLOCK_BYTES; i++) {
cipher.processBlock(counter, 0, newKey, i * BLOCK_BYTES);
incrementCounter();
}
System.arraycopy(newKey, 0, key, 0, KEY_BYTES);
for(int i = 0; i < KEY_BYTES; i++) newKey[i] = 0;
// Return the number of bytes written
return len;
}
// Generate a partial block if needed
int done = fullBlocks * BLOCK_BYTES, remaining = len - done;
assert remaining < BLOCK_BYTES;
if(remaining > 0) {
cipher.processBlock(counter, 0, buffer, 0);
incrementCounter();
// Copy the partial block to the output buffer and erase our copy
System.arraycopy(buffer, 0, dest, off + done, remaining);
for(int i = 0; i < BLOCK_BYTES; i++) buffer[i] = 0;
}
// Generate a new key
for(int i = 0; i < KEY_BYTES / BLOCK_BYTES; i++) {
cipher.processBlock(counter, 0, newKey, i * BLOCK_BYTES);
incrementCounter();
finally{
synchLock.unlock();
}
System.arraycopy(newKey, 0, key, 0, KEY_BYTES);
for(int i = 0; i < KEY_BYTES; i++) newKey[i] = 0;
// Return the number of bytes written
return len;
}
}
package org.briarproject.crypto;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import org.briarproject.api.crypto.MessageDigest;
import org.briarproject.api.crypto.PseudoRandom;
import org.briarproject.util.ByteUtils;
......@@ -10,6 +13,8 @@ class PseudoRandomImpl implements PseudoRandom {
private byte[] state;
private int offset;
private final Lock synchLock = new ReentrantLock();
PseudoRandomImpl(MessageDigest messageDigest, int seed1, int seed2) {
this.messageDigest = messageDigest;
......@@ -21,21 +26,27 @@ class PseudoRandomImpl implements PseudoRandom {
offset = 0;
}
public synchronized byte[] nextBytes(int bytes) {
byte[] b = new byte[bytes];
int half = state.length / 2;
int off = 0, len = b.length, available = half - offset;
while(available < len) {
System.arraycopy(state, offset, b, off, available);
off += available;
len -= available;
messageDigest.update(state, half, half);
state = messageDigest.digest();
offset = 0;
available = half;
public byte[] nextBytes(int bytes) {
synchLock.lock();
try{
byte[] b = new byte[bytes];
int half = state.length / 2;
int off = 0, len = b.length, available = half - offset;
while(available < len) {
System.arraycopy(state, offset, b, off, available);
off += available;
len -= available;
messageDigest.update(state, half, half);
state = messageDigest.digest();
offset = 0;
available = half;
}
System.arraycopy(state, offset, b, off, len);
offset += len;
return b;
}
finally{
synchLock.unlock();
}
System.arraycopy(state, offset, b, off, len);
offset += len;
return b;
}
}
package org.briarproject.crypto;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import org.briarproject.api.crypto.SecretKey;
import org.briarproject.util.ByteUtils;
......@@ -8,23 +11,38 @@ class SecretKeyImpl implements SecretKey {
private final byte[] key;
private boolean erased = false; // Locking: this
private final Lock synchLock = new ReentrantLock();
SecretKeyImpl(byte[] key) {
this.key = key;
}
public synchronized byte[] getEncoded() {
if(erased) throw new IllegalStateException();
return key;
public byte[] getEncoded() {
synchLock.lock();
try{
if(erased) throw new IllegalStateException();
return key;
}
finally{
synchLock.unlock();
}
}
public SecretKey copy() {
return new SecretKeyImpl(key.clone());
}
public synchronized void erase() {
if(erased) throw new IllegalStateException();
ByteUtils.erase(key);
erased = true;
public void erase() {
synchLock.lock();
try{
if(erased) throw new IllegalStateException();
ByteUtils.erase(key);
erased = true;
}
finally{
synchLock.unlock();
}
}
}
......@@ -27,6 +27,9 @@ import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Logger;
import org.briarproject.api.Author;
......@@ -322,6 +325,9 @@ abstract class JdbcDatabase implements Database<Connection> {
protected abstract Connection createConnection() throws SQLException;
protected abstract void flushBuffersToDisk(Statement s) throws SQLException;
private final Lock connectionsLock = new ReentrantLock();
private final Condition connectionsChanged = connectionsLock.newCondition();
JdbcDatabase(String hashType, String binaryType, String counterType,
String secretType, Clock clock) {
......@@ -431,19 +437,28 @@ abstract class JdbcDatabase implements Database<Connection> {
public Connection startTransaction() throws DbException {
Connection txn = null;
synchronized(connections) {
connectionsLock.lock();
try {
if(closed) throw new DbClosedException();
txn = connections.poll();
}
finally{
connectionsLock.unlock();
}
try {
if(txn == null) {
// Open a new connection
txn = createConnection();
if(txn == null) throw new DbException();
txn.setAutoCommit(false);
synchronized(connections) {
connectionsLock.lock();
try {
openConnections++;
}
finally{
connectionsLock.unlock();
}
}
} catch(SQLException e) {
throw new DbException(e);
......@@ -455,9 +470,13 @@ abstract class JdbcDatabase implements Database<Connection> {
public void abortTransaction(Connection txn) {
try {
txn.rollback();
synchronized(connections) {
connectionsLock.lock();
try {
connections.add(txn);
connections.notifyAll();
connectionsChanged.signalAll();
}
finally{
connectionsLock.unlock();
}
} catch(SQLException e) {
// Try to close the connection
......@@ -468,11 +487,14 @@ abstract class JdbcDatabase implements Database<Connection> {
if(LOG.isLoggable(WARNING)) LOG.log(WARNING, e1.toString(), e1);
}
// Whatever happens, allow the database to close
synchronized(connections) {
connectionsLock.lock();
try {
openConnections--;
connections.notifyAll();
connectionsChanged.signalAll();
}
}
finally{
connectionsLock.unlock();
} }
}
public void commitTransaction(Connection txn) throws DbException {
......@@ -486,9 +508,13 @@ abstract class JdbcDatabase implements Database<Connection> {
tryToClose(s);
throw new DbException(e);
}
synchronized(connections) {
connectionsLock.lock();
try{
connections.add(txn);
connections.notifyAll();
connectionsChanged.signalAll();
}
finally{
connectionsLock.unlock();
}
}
......@@ -502,14 +528,15 @@ abstract class JdbcDatabase implements Database<Connection> {
protected void closeAllConnections() throws SQLException {
boolean interrupted = false;
synchronized(connections) {
connectionsLock.lock();
try{
closed = true;
for(Connection c : connections) c.close();
openConnections -= connections.size();
connections.clear();
while(openConnections > 0) {
try {