Skip to content

Commit

Permalink
Fix #5232: ConsoleLogger overwrites local log file for each line write (
Browse files Browse the repository at this point in the history
#5550)

## Explanation
Fix #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]>
  • Loading branch information
manas-yu and adhiamboperes authored Nov 11, 2024
1 parent 32077ab commit 96f5289
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import org.oppia.android.app.model.EventLog.ConsoleLoggerContext
import org.oppia.android.util.locale.OppiaLocale
import org.oppia.android.util.threading.BlockingDispatcher
import java.io.File
import java.io.FileWriter
import java.io.PrintWriter
import javax.inject.Inject
import javax.inject.Singleton

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

private var printWriter: PrintWriter? = null

/** Logs a verbose message with the specified tag. */
fun v(tag: String, msg: String) {
writeLog(LogLevel.VERBOSE, tag, msg)
Expand Down Expand Up @@ -73,12 +77,12 @@ class ConsoleLogger @Inject constructor(
writeError(LogLevel.WARNING, tag, msg, tr)
}

/** Logs a error message with the specified tag. */
/** Logs an error message with the specified tag. */
fun e(tag: String, msg: String) {
writeLog(LogLevel.ERROR, tag, msg)
}

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

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

/**
* Writes the specified text line to file in a background thread to ensure that saving messages don't block the main
* thread. A blocking dispatcher is used to ensure messages are written in order.
* Writes the specified text line to file in a background thread to ensure that saving messages
* doesn't block the main thread. A blocking dispatcher is used to ensure messages are written
* in order.
*/
private fun logToFileInBackground(text: String) {
blockingScope.launch { logDirectory.printWriter().use { out -> out.println(text) } }
blockingScope.launch {
if (printWriter == null) {
printWriter = PrintWriter(FileWriter(logDirectory, true)) // Open in append mode.
}
printWriter?.println(text)
printWriter?.flush()
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.async
import kotlinx.coroutines.flow.take
import kotlinx.coroutines.flow.toList
import org.junit.After
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
Expand All @@ -31,6 +32,8 @@ import org.oppia.android.util.data.DataProvidersInjectorProvider
import org.oppia.android.util.locale.testing.LocaleTestModule
import org.robolectric.annotation.Config
import org.robolectric.annotation.LooperMode
import java.io.File
import java.io.PrintWriter
import javax.inject.Inject
import javax.inject.Singleton

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

private lateinit var logFile: File

@Before
fun setUp() {
setUpTestApplicationComponent()
logFile = File(context.filesDir, "oppia_app.log")
logFile.delete()
}

@After
fun tearDown() {
logFile.delete()
}

@Test
fun testConsoleLogger_multipleLogCalls_appendsToFile() {
consoleLogger.e(testTag, testMessage)
consoleLogger.e(testTag, "$testMessage 2")
testCoroutineDispatchers.advanceUntilIdle()

val logContent = logFile.readText()
assertThat(logContent).contains(testMessage)
assertThat(logContent).contains("$testMessage 2")
assertThat(logContent.indexOf(testMessage))
.isLessThan(logContent.indexOf("$testMessage 2"))
}

@Test
Expand All @@ -76,6 +101,26 @@ class ConsoleLoggerTest {
assertThat(firstErrorContext.fullErrorLog).isEqualTo(testMessage)
}

@Test
fun testConsoleLogger_closeAndReopen_continuesToAppend() {
consoleLogger.e(testTag, "first $testMessage")
testCoroutineDispatchers.advanceUntilIdle()

// Force close the PrintWriter to simulate app restart
val printWriterField = ConsoleLogger::class.java.getDeclaredField("printWriter")
printWriterField.isAccessible = true
(printWriterField.get(consoleLogger) as? PrintWriter)?.close()
printWriterField.set(consoleLogger, null)

consoleLogger.e(testTag, "first $testMessage")
consoleLogger.e(testTag, "second $testMessage")
testCoroutineDispatchers.advanceUntilIdle()

val logContent = logFile.readText()
assertThat(logContent).contains("first $testMessage")
assertThat(logContent).contains("second $testMessage")
}

private fun setUpTestApplicationComponent() {
ApplicationProvider.getApplicationContext<TestApplication>().inject(this)
}
Expand All @@ -84,9 +129,7 @@ class ConsoleLoggerTest {
class TestModule {
@Provides
@Singleton
fun provideContext(application: Application): Context {
return application
}
fun provideContext(application: Application): Context = application

@Provides
@Singleton
Expand All @@ -96,7 +139,7 @@ class ConsoleLoggerTest {
@Provides
@Singleton
@EnableFileLog
fun provideEnableFileLog(): Boolean = false
fun provideEnableFileLog(): Boolean = true

@Provides
@Singleton
Expand All @@ -113,7 +156,6 @@ class ConsoleLoggerTest {
FakeOppiaClockModule::class,
]
)

interface TestApplicationComponent : DataProvidersInjector {
@Component.Builder
interface Builder {
Expand Down

0 comments on commit 96f5289

Please sign in to comment.