Closing FileLogWriter

Question

Hi,

I'm using Sftp with a FileLogWriter. When FileLogWriter is opened, it obtains a lock on the log file which persists until it is closed. The problem is that this does not happen when the associated Sftp is disposed; also, I need to cast LogWriter to access Close(). This means that at some point before Sftp is disposed, I have to do something like:

if (sftp.LogWriter != null)
    ((FileLogWriter)sftp.LogWriter).Close();

It would be nice if (a) ILogWriter implemented IDisposable, so that all LogWriter implementations could be disposed without casting, and (b) NetworkSession also implemented IDisposable, with Dispose taking care of cleaning up the LogWriter.

Thanks, Aaron.

Answer 1

Point (a) - on the one side, adding IDisposable to ILogWriter is not logically correct. It should define just only logging interface. If there were a class eg. StringLogWriter the IDisposable is meaningless for this class.
On the other side, logging into closeable media (such as file) is the most probable case, so adding IDisposable would be handy.

We will discuss it more internally, however extending an interface is breaking change, so I think the IDisposable is not going to be added into the ILogWriter interface.

I can offer you a workaround using Extension methods:

public static class MyExtensions
{
    public static void Close(this ILogWriter logWriter)
    {
        IDisposable disposable = logWriter as IDisposable;
        if (disposable != null)
            disposable.Dispose();
    }
}

// usage:
client.LogWriter = new FileLogWriter(...);
// do some work ...
client.Disconnect();
client.LogWriter.Close();

Point (b) - closing LogWriter in the NetworkSession.Dispose() method is also not a case, because LogWriter can be shared between NetworkSessions. It is due to ability to log multiple sessions into a single media. If the LogWriter is closed automatically when one NetworkSession is closed, other sessions with shared LogWriter fail to log their state.

But if this is not your case there is also workaround using Dispose pattern:

public class MySftp : Sftp
{
    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            IDisposable disposable = LogWriter as IDisposable;
            if (disposable != null)
                disposable.Dispose();
        }
        base.Dispose(disposing);
    }
}

// usage:
using (MySftp client = new MySftp())
{
    // do some work...
}

Answer 2

Thanks for the detailed response. I see your point about IDisposable not being relevant to a string LogWriter, but I believe there are other instances in .NET of classes implementing IDisposable when it's not strictly necessary -- MemoryStream, for example, needs to implement IDisposable because it descends from Stream, but MemoryStream.Dispose() doesn't actually clean up any resources. I also see what you mean about LogWriter being shared between multiple sessions; perhaps there could be some kind of reference counting implemented? I haven't thought this out, but I still think it would be nice for LogWriter to somehow get automatically cleaned up.

Another way this could be handled would be for FileLogWriter to have an option not to keep the file open; NLog works this way. You incur the overhead of having to open the file before each write and close it afterwards, but then you wouldn't need to Dispose because the file is closed anyway. You could also then have multiple FileLogWriters writing to the same file, rather than sharing one instance among multiple clients.

I hadn't thought about the extension method approach, but the Dispose pattern is exactly what I had implemented, since I'm not sharing LogWriters between clients.

Answer 3

Thanks for your suggestions!

1) It's true that it's inconvenient that the log file is not closed when an instance of Sftp is disposed even if it's the only object using the FileLogWriter. We will keep this in mind and try to come up with a better solution in one of the next releases.

2) As Lukas said, we are not sure about IDisposable in this case. If we added it to ILogWriter, every implementation would have to provide one, even though they might not need it or want to make their logging objects disposable this way. Adding it would also affect existing implementations - every customer who has implemented his own log writer would get a compile-time error after upgrading to a new release. Still, we will discuss this more thoroughly once we are done with the next release.

3) Reference counting (perhaps limited to FileLogWriter) sounds like an interesting idea, but perhaps too complicated. Adding an option to FileLogWriter that says it's bound to the first object it has been assigned to (and automatically disposed with it) might be sufficient in most cases. Adding an option to Sftp that says it should dispose the LogWriter object on Dispose is another possibility.

4) An option not to keep the file open sounds like a nice feature to have. But interestingly, you can have multiple FileLogWriters writing to the same file at the moment - we use FileMode.Append and FileAccess.Write when opening it.

Comments

Thanks again for the detailed and thoughtful response!