Skip to content

Commit ec3f615

Browse files
manas-yuadhiamboperes
authored andcommitted
Fix oppia#5232: ConsoleLogger overwrites local log file for each line write (oppia#5550)
## Explanation Fix oppia#5232 This PR updates the `ConsoleLogger` class to improve logging functionality. The key changes include: - **Append Mode for Logs**: The `FileWriter` is now opened in append mode, ensuring that new log entries are added to the existing log file instead of overwriting it. - **Long-lived PrintWriter**: A long-lived `PrintWriter` instance has been implemented to enhance performance by reducing the overhead of opening and closing the log file multiple times. - **Close Log File**: A `closeLogFile()` method has been added to properly close the `PrintWriter`, preventing resource leaks after logging is complete. These changes address the issue of logs being overwritten and enhance performance while ensuring proper resource management. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). --------- Co-authored-by: Adhiambo Peres <[email protected]>
1 parent 2fdb5c6 commit ec3f615

File tree

2 files changed

+64
-11
lines changed

2 files changed

+64
-11
lines changed

utility/src/main/java/org/oppia/android/util/logging/ConsoleLogger.kt

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import org.oppia.android.app.model.EventLog.ConsoleLoggerContext
1111
import org.oppia.android.util.locale.OppiaLocale
1212
import org.oppia.android.util.threading.BlockingDispatcher
1313
import java.io.File
14+
import java.io.FileWriter
15+
import java.io.PrintWriter
1416
import javax.inject.Inject
1517
import javax.inject.Singleton
1618

@@ -33,6 +35,8 @@ class ConsoleLogger @Inject constructor(
3335
*/
3436
val logErrorMessagesFlow: SharedFlow<ConsoleLoggerContext> = _logErrorMessagesFlow
3537

38+
private var printWriter: PrintWriter? = null
39+
3640
/** Logs a verbose message with the specified tag. */
3741
fun v(tag: String, msg: String) {
3842
writeLog(LogLevel.VERBOSE, tag, msg)
@@ -73,12 +77,12 @@ class ConsoleLogger @Inject constructor(
7377
writeError(LogLevel.WARNING, tag, msg, tr)
7478
}
7579

76-
/** Logs a error message with the specified tag. */
80+
/** Logs an error message with the specified tag. */
7781
fun e(tag: String, msg: String) {
7882
writeLog(LogLevel.ERROR, tag, msg)
7983
}
8084

81-
/** Logs a error message with the specified tag, message and exception. */
85+
/** Logs an error message with the specified tag, message and exception. */
8286
fun e(tag: String, msg: String, tr: Throwable?) {
8387
writeError(LogLevel.ERROR, tag, msg, tr)
8488
}
@@ -109,7 +113,7 @@ class ConsoleLogger @Inject constructor(
109113
}
110114

111115
// Add the log to the error message flow so it can be logged to firebase.
112-
CoroutineScope(blockingDispatcher).launch {
116+
blockingScope.launch {
113117
// Only log error messages to firebase.
114118
if (logLevel == LogLevel.ERROR) {
115119
_logErrorMessagesFlow.emit(
@@ -124,10 +128,17 @@ class ConsoleLogger @Inject constructor(
124128
}
125129

126130
/**
127-
* Writes the specified text line to file in a background thread to ensure that saving messages don't block the main
128-
* thread. A blocking dispatcher is used to ensure messages are written in order.
131+
* Writes the specified text line to file in a background thread to ensure that saving messages
132+
* doesn't block the main thread. A blocking dispatcher is used to ensure messages are written
133+
* in order.
129134
*/
130135
private fun logToFileInBackground(text: String) {
131-
blockingScope.launch { logDirectory.printWriter().use { out -> out.println(text) } }
136+
blockingScope.launch {
137+
if (printWriter == null) {
138+
printWriter = PrintWriter(FileWriter(logDirectory, true)) // Open in append mode.
139+
}
140+
printWriter?.println(text)
141+
printWriter?.flush()
142+
}
132143
}
133144
}

utility/src/test/java/org/oppia/android/util/logging/ConsoleLoggerTest.kt

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi
1313
import kotlinx.coroutines.async
1414
import kotlinx.coroutines.flow.take
1515
import kotlinx.coroutines.flow.toList
16+
import org.junit.After
1617
import org.junit.Before
1718
import org.junit.Test
1819
import org.junit.runner.RunWith
@@ -31,6 +32,8 @@ import org.oppia.android.util.data.DataProvidersInjectorProvider
3132
import org.oppia.android.util.locale.testing.LocaleTestModule
3233
import org.robolectric.annotation.Config
3334
import org.robolectric.annotation.LooperMode
35+
import java.io.File
36+
import java.io.PrintWriter
3437
import javax.inject.Inject
3538
import javax.inject.Singleton
3639

@@ -54,9 +57,31 @@ class ConsoleLoggerTest {
5457
@field:[Inject BackgroundTestDispatcher]
5558
lateinit var backgroundTestDispatcher: TestCoroutineDispatcher
5659

60+
private lateinit var logFile: File
61+
5762
@Before
5863
fun setUp() {
5964
setUpTestApplicationComponent()
65+
logFile = File(context.filesDir, "oppia_app.log")
66+
logFile.delete()
67+
}
68+
69+
@After
70+
fun tearDown() {
71+
logFile.delete()
72+
}
73+
74+
@Test
75+
fun testConsoleLogger_multipleLogCalls_appendsToFile() {
76+
consoleLogger.e(testTag, testMessage)
77+
consoleLogger.e(testTag, "$testMessage 2")
78+
testCoroutineDispatchers.advanceUntilIdle()
79+
80+
val logContent = logFile.readText()
81+
assertThat(logContent).contains(testMessage)
82+
assertThat(logContent).contains("$testMessage 2")
83+
assertThat(logContent.indexOf(testMessage))
84+
.isLessThan(logContent.indexOf("$testMessage 2"))
6085
}
6186

6287
@Test
@@ -76,6 +101,26 @@ class ConsoleLoggerTest {
76101
assertThat(firstErrorContext.fullErrorLog).isEqualTo(testMessage)
77102
}
78103

104+
@Test
105+
fun testConsoleLogger_closeAndReopen_continuesToAppend() {
106+
consoleLogger.e(testTag, "first $testMessage")
107+
testCoroutineDispatchers.advanceUntilIdle()
108+
109+
// Force close the PrintWriter to simulate app restart
110+
val printWriterField = ConsoleLogger::class.java.getDeclaredField("printWriter")
111+
printWriterField.isAccessible = true
112+
(printWriterField.get(consoleLogger) as? PrintWriter)?.close()
113+
printWriterField.set(consoleLogger, null)
114+
115+
consoleLogger.e(testTag, "first $testMessage")
116+
consoleLogger.e(testTag, "second $testMessage")
117+
testCoroutineDispatchers.advanceUntilIdle()
118+
119+
val logContent = logFile.readText()
120+
assertThat(logContent).contains("first $testMessage")
121+
assertThat(logContent).contains("second $testMessage")
122+
}
123+
79124
private fun setUpTestApplicationComponent() {
80125
ApplicationProvider.getApplicationContext<TestApplication>().inject(this)
81126
}
@@ -84,9 +129,7 @@ class ConsoleLoggerTest {
84129
class TestModule {
85130
@Provides
86131
@Singleton
87-
fun provideContext(application: Application): Context {
88-
return application
89-
}
132+
fun provideContext(application: Application): Context = application
90133

91134
@Provides
92135
@Singleton
@@ -96,7 +139,7 @@ class ConsoleLoggerTest {
96139
@Provides
97140
@Singleton
98141
@EnableFileLog
99-
fun provideEnableFileLog(): Boolean = false
142+
fun provideEnableFileLog(): Boolean = true
100143

101144
@Provides
102145
@Singleton
@@ -113,7 +156,6 @@ class ConsoleLoggerTest {
113156
FakeOppiaClockModule::class,
114157
]
115158
)
116-
117159
interface TestApplicationComponent : DataProvidersInjector {
118160
@Component.Builder
119161
interface Builder {

0 commit comments

Comments
 (0)