Is it a bad idea to put frequent file I/O operations within a SyncLock block?

Question

Say I have some code that does this:

Public Function AppendToLogFile(ByVal s As String) As Boolean
    Dim success As Boolean = True
    Dim fs As IO.FileStream = Nothing
    Dim sw As IO.StreamWriter = Nothing

    Static LogFileLock As New Object()
    SyncLock LogFileLock
        Try
            fs = New IO.FileStream(LogFilePath)
            sw = New IO.StreamWriter(fs)
            sw.WriteLine(s)

        Catch ex As Exception
            success = False

        Finally
            If Not sw Is Nothing Then sw.Close()
            If Not fs Is Nothing Then fs.Close()
        End Try
    End SyncLock

    Return success
End Function

First of all: is it a problem that I have that Try/Catch/Finally block inside of a SyncLock?

Second of all: suppose this code runs, on an event, potentially many times within a small timeframe--say, ten times in one second. Is it OK to have it SyncLock like this, or would it make more sense to have it add a line to a Queue, and then write all the lines from the Queue to the file on a timer that goes off, say, every second?

Answer 1

That looks okay to me at first glance, with two caveats:

1. Static members use a kind of thread-safe locking already behind the scenes. So rather than an explicit lock you might just be able to piggyback on the existing lock. I'm not exactly sure what that would look like, though.
2. Don't return status codes. Let the exception propagate up to the appropriate level. Once you do that, you can re-write your code like this:

.

Public Sub AppendToLogFile(ByVal s As String) As Boolean
    Static LogFileLock As New Object()
    SyncLock LogFileLock
        Using sw As New IO.StreamWriter(LogFilePath)
            sw.WriteLine(s)
        End Using
    End SyncLock
End Sub

That's all the functionality in less than half the code. The only difference is that you have to handle the exception in the calling code rather than check the return status.

Answer 2

In your case, the locking is fine as long as the log file is written to relatively infrequently. In other words, if every successful operation is written to the log, it might not be a good design. If only failed operations are writing to the log, then it might be more than adequate.

Moreover, if you are writing to this log frequently, you probably want to reference the `StreamWriter' in a shared variable.

Public NotInheritable Class Log

    Private Shared m_LogLock As New Object
    Private Shared m_Log As StreamWriter

    Public Shared Sub WriteLog(ByVal message As String)
        SyncLock m_LogLock
            If m_Log Is Nothing Then
                m_Log = New StreamWriter("pathAndFileName")
            End If
            m_Log.WriteLine(message)
            m_Log.Flush()
        End SyncLock
    End Sub

End Class