Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have created a database logger for ASP:NET Core's logging API and I'd like to publish it on NuGet when ready. The GitHub repository is here.

The library is partly modeled after Microsoft.Extensions.Logging.Console and I tried to follow good practices of object-oriented design.

I am slightly concerned about some decisions I made while implementing ILogWriter interface in SqlServerLogWriter class AND while designing the interface for the WriteBulk method (the method is called from the LogRecordCache class).

  1. Have I managed threading well enough? It works fine, but it might not look OK.
  2. Should I hide exceptions or let them propagate? Logging should be redundant in production environment, so I think hiding them would be fine.
  3. Is there a better way for column mapping (in SqlServerLogWriter)?
  4. Any advice or contribution would be appreciated.

LogRecordCache.cs

internal static class LogRecordCache
{
    private static readonly List<LogRecord> _logs;
    private static bool _flushingInProgress = false;
    private static object _lockObject = new object();

    public static bool IsEmpty { get { return _flushingInProgress || _logs.Count == 0; } }


    static LogRecordCache()
    {
        _logs = new List<LogRecord>();
    }


    public static void Add(LogRecord log)
    {
        if (_flushingInProgress)
        {
            lock (_lockObject)
            {
                _logs.Add(log);
            }
        }
        else
        {
            _logs.Add(log);
        }
    }

    public static bool IsFull(int maxCount)
    {
        return !_flushingInProgress && _logs.Count >= maxCount;
    }

    public static void Flush(ILogWriter writer)
    {
        if (!_flushingInProgress)
        {
            _flushingInProgress = true;
            Task.Run(() => writer.WriteBulk(_logs, _lockObject, ref _flushingInProgress));
        }
    }
}

WriteBulk() method

/// <summary>
/// Writes a collection of log records to database at once
/// </summary>
/// <param name="logs">List of <see cref="LogRecord"/> objects</param>
/// <param name="lockObject">Locking object</param>
/// <param name="flushingInProgress">Indicates if writing process is still in progress; set to false at the end of this method</param>
/// <exception cref="SqlException">If opening a connection through the provided connection string was not possible</exception>
public void WriteBulk(List<LogRecord> logs, object lockObject, ref bool flushingInProgress)
{
    var lockTaken = false;
    var connection = new SqlConnection(_connectionString);
    connection.Open();
    var transaction = connection.BeginTransaction();

    try
    {
        using (var bulkCopy = new SqlBulkCopy(connection, SqlBulkCopyOptions.TableLock, transaction))
        {
            bulkCopy.DestinationTableName = "Logging";
            foreach (var mapping in _columnMappings)
                bulkCopy.ColumnMappings.Add(mapping.Key, mapping.Value);

            Monitor.TryEnter(lockObject, ref lockTaken);
            if (lockTaken)
            {
                using (var reader = ObjectReader.Create(logs, _columnMappings.Keys.ToArray()))
                {
                    bulkCopy.BatchSize = logs.Count;
                    bulkCopy.WriteToServer(reader);
                }

                transaction.Commit();
            }
        }
    }
    catch
    {
        if (connection.State == ConnectionState.Open)
            transaction.Rollback();
        throw;
    }
    finally
    {
        if (lockTaken)
        {
            logs.Clear();
            Monitor.Exit(lockObject);
        }

        transaction.Dispose();
        connection.Close();
        flushingInProgress = false;
    }
}
share|improve this question

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.