Synchronize writing thread with existing writers #634
Conversation
manisiu
force-pushed
the
v2.7
branch
2 times, most recently
from
06ae51a to
5f42c07
Compare
|
Thank you for your PR! I like the approach and would like to release it as tinylog 2.7-RC2. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## v2.7 #634 +/- ##
============================================
- Coverage 94.33% 94.29% -0.04%
Complexity 2829 2829
============================================
Files 149 149
Lines 5576 5577 +1
Branches 722 722
============================================
- Hits 5260 5259 -1
- Misses 176 178 +2
Partials 140 140 ☔ View full report in Codecov by Sentry. |
|
I think we have to use only one mutex. Currently, the logging benchmarks are running into a race condition. With these adjustments, your approach is working for without any race condition and the same throughput as the original implementation: public final class WritingThread extends Thread {
private static final String THREAD_NAME = "tinylog-WritingThread";
private final Object mutex;
private final Collection<Writer> writers;
private List<Task> tasks;
WritingThread(final Collection<Writer> writers) {
this.mutex = new Object();
this.writers = writers;
this.tasks = new ArrayList<Task>();
setName(THREAD_NAME);
setPriority(Thread.MIN_PRIORITY);
setDaemon(true);
}
@Override
public void run() {
Collection<Writer> writers = new ArrayList<Writer>(1);
while (true) {
for (Task task : receiveTasks()) {
if (task == Task.POISON) {
close();
return;
} else {
write(writers, task);
}
}
flush(writers);
writers.clear();
}
}
public void add(final Writer writer, final LogEntry logEntry) {
Task task = new Task(writer, logEntry);
synchronized (mutex) {
tasks.add(task);
mutex.notify();
}
}
public void shutdown() {
synchronized (mutex) {
tasks.add(Task.POISON);
mutex.notify();
}
}
private List<Task> receiveTasks() {
synchronized (mutex) {
while (true) {
if (tasks.isEmpty()) {
try {
mutex.wait();
} catch (InterruptedException ex) {
return Collections.emptyList();
}
} else {
List<Task> currentTasks = tasks;
tasks = new ArrayList<Task>();
return currentTasks;
}
}
}
}
[...]
}[...]
<Match>
<!-- Writing Thread -->
<Class name="org.tinylog.core.WritingThread" />
<!-- There is only one writhing thread instance to wake up -->
<Bug pattern="NO_NOTIFY_NOT_NOTIFYALL" />
</Match>
[...]
@manisiu Can you have a look? What do you think? |
|
Thanks for the quick reply and happy holidays! I'm curious regarding the race condition you mentioned - is there a test I can run to see this in action? As far as I can tell the It probably makes sense anyway to use only one mutex as it would make it clearer what's happening and the performance hit is negligible (if any at all). The code looks good to me, Maybe I'd just simplify I now realised my conditional flushing logic wasn't necessary since the writers are cleared after each iteration, so thanks for getting rid of that. I can update the PR with these changes if that works unless you want to add the changes in yourself. Cheers! |
You can execute the logging benchmark to run into the race condition. After installing all tinylog artifact via Maven, you can execute the tinylog benchmarks via the Maven command
It happens because it is not guaranteed that the writing thread is already in the waiting state when signaling the thread to wake up.
I like your simplification! You are welcome to update the PR. Thank you very much for your great contribution! It is really a great improvement for asynchronous logging. |
|
BTW - running I get this with both the maven release in the Linux Mint repos (so Ubuntu) and the latest 3.9,6 binary. I don't usually program in Java, so there might be something obvious I'm missing, but this error seems to say we're accessing some non-public fields through reflection? |
This error usually pops up when Maven uses Java 11 or later. You can execute |
It's definitely JDK 9, I remember having different issues with Java 11. Could it be you're using slightly older versions and some defaults have changed? |
I see you use OpenJ9. Unfortunately, I have never tested OpenJ9. With OpenJDK 9 it should work. |
|
I'm happy to merge your changes! Thank you for the great contribution. I will release tinylog 2.7 RC2 within the next days. Wish you all the best for 2024! |
Description
The writing thread on Android apps can prevent the app from going to sleep as it constantly wakes up every 10ms. I've added synchronisation to wake up only when something is being written. This is critical for our current use case.