[RollingFile and RollingXML Listeners] Reinstantiate underlying FileStream on a write error

[alex-grigoras]

Hi,

Thanks for the listeners, good stuff!

However, if you have a rolling file listener (plain text or xml) that writes to a drive that can have transient faults (network drive that has temporary network error or usb stick that is unplugged and plugged back in) the listener does not work after such a transient fault even if the fault goes away. This is because the underlying FileStream becomes invalid after the fault and there is no mechanism to have it instantiated again.

I added a config attribute called NewStreamOnError for the RollingFile and RollingXML listeners. When it is set to false everything works like before. When you set it to True it causes the underlying FileStream to be reinstantiated if an error (any Exception) occurs when calling Write, WriteLine or Flush on the underlying FileStream. When the error occurs the FileStream reference is set to null and it is instantiated again on the next logging call.

Example listener config:

<add name="rollingFile"
        type="Essential.Diagnostics.RollingFileTraceListener, Essential.Diagnostics.RollingFileTraceListener"
        initializeData="share\alex\loggy"
        template="{LocalDateTime:yyyy-MM-dd HH:mm:ss.fff} [{Thread}] {EventType} {Source} {Id}: {Message}{Data}"
        newStreamOnError="True" />

[sgryphon]

This is a good change, and I like the idea, but would like a few changes as outlined above.

Mostly, it is rather than pass the same flag to every call (write, flush, etc), pass it in the constructor of RollingTextWriter... and to do this the creation of the writer needs to be lazy, done on first access. I've suggested it as a property getter, but you could also implement it as an EnsureRollingTextWriter() method, similar to within the writer (where it calls EnsureCurrentWriter).

[sgryphon]

This is a good change, and I like the idea, but would like a few changes as outlined above.

Mostly, it is rather than pass the same flag to every call (write, flush, etc), pass it in the constructor of RollingTextWriter... and to do this the creation of the writer needs to be lazy, done on first access. I've suggested it as a property getter, but you could also implement it as an EnsureRollingTextWriter() method, similar to within the writer (where it calls EnsureCurrentWriter).

[alex-grigoras]

Hi, thank you for the feedback and sorry for the delay, the messages ended up in my spam folder.
I didn't originally set the property in the constructor because the Attributes collection is populated at a later time (https://stackoverflow.com/questions/11559916/loading-trace-listener-attributes).

By lazy creating the text writer I can get around this problem, good call. I will update the code.

[alex-grigoras]

Now that I looked at it some more I think that making the RollingTextWriter lazy-initialized may cause some subtle bugs in the future.

If this property is ever used in the RollingFileTL and RollingXMLTL constructors, where the Attributes collection is not yet populated, it means that the NewStreamOnError property will always have it's default value no matter what it's set to in the config file.

I have a commit ready but not pushed and everything works fine, for now, but if some time in the future a developer accesses RollingTextWriter.Something in the 2 trace listener's constructors it will cause RollingTextWriter to be initialized when the Attributes are empty.

I'm ok with that if you are :) ...but I'd personally take the annoyance of passing an extra parameter to Write and Flush over future easy-to-introduce-hard-to-see-bugs.

What do you think works best here?

[sgryphon]

If this property is ever used in the RollingFileTL and RollingXMLTL constructors, where the Attributes collection is not yet populated, it means that the NewStreamOnError property will always have it's default value no matter what it's set to in the config file.

What do you think works best here?

The constructors are part of this overall project, so won't suddenly change to access the properties, etc.

The pattern where the initializeData is passed into the constructor, and then all other properties are configured via property setters, is baked into the way trace listeners work. Hence things like returning the collection of valid property names.

The issue about accessing during the constructor applies not just to this property, but to every custom property on every trace listener, i.e. it is a well known limitation. (And some of those properties would potentially cause a lot more issues than simply setting this flag.)

If it was generic code, sure, we need to be careful of such things; but specifically for a trace listener it is not an issue, because every property is set via that mechanism.

[alex-grigoras]

Fair enough. I made that object be lazy initialized. I also checked the docs on bool.TryParse and it looks to me like the result is set to false if the conversion fails. MSDN link

[alex-grigoras]

Ping!