Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #23 workaround #24

Merged
merged 2 commits into from
Nov 14, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Serilog.Sinks.File/Sinks/File/WriteCountingStream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public override void Write(byte[] buffer, int offset, int count)

public override void Flush() => _stream.Flush();
public override bool CanRead => false;
public override bool CanSeek => false;
public override bool CanSeek => _stream.CanSeek;
public override bool CanWrite => true;
public override long Length => _stream.Length;

Expand All @@ -60,7 +60,7 @@ public override long Position

public override long Seek(long offset, SeekOrigin origin)
{
throw new NotSupportedException();
return _stream.Seek(offset, origin);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the workaround be impacted if we continue to throw here? Although it appears to break the CanSeek/Seek() contract, actually calling Seek() would break the write counting. Perhaps as a compromise we could throw InvalidOperationException() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the WriteCountingStream actually necessary at all? For what I can see, being a private sealed class with only a single usage within the project, it exists only to support a file size limit. In this case it seems to me that a regular FIleStream would be sufficient by using the Length property and more robust to boot. No functionality of the actual stream would need to be wrapped and turned of either, like Seek. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good question. FileStream.Length turns out to be a couple of orders of magnitude slower, as it calls through to get the file size from the OS (at least, that's my recollection from testing it).

Open to scrapping it, but would need to re-do benchmarking. There's a small sample app in the solution that gives some basic write throughput numbers if you're keen to check it out - I'll have another look when I get a chance 👍

}

public override void SetLength(long value)
Expand Down
71 changes: 71 additions & 0 deletions test/Serilog.Sinks.File.Tests/FileSinkTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
using Serilog.Formatting.Json;
using Serilog.Sinks.File.Tests.Support;
using Serilog.Tests.Support;
using System.Text;
using Serilog.Tests;

namespace Serilog.Sinks.File.Tests
{
Expand Down Expand Up @@ -99,6 +101,75 @@ public void WhenLimitIsNotSpecifiedFileSizeIsNotRestricted()
Assert.True(size > maxBytes * 2);
}
}


[Fact]
public void WhenLimitIsSpecifiedAndEncodingHasPreambleDataIsCorrectlyAppendedToFileSink()
{
long? maxBytes = 5000;
var encoding = Encoding.UTF8;

Assert.True(encoding.GetPreamble().Length > 0);
WriteTwoEventsAndCheckOutputFileLength(maxBytes, encoding);
}

[Fact]
public void WhenLimitIsNotSpecifiedAndEncodingHasPreambleDataIsCorrectlyAppendedToFileSink()
{
long? maxBytes = null;
var encoding = Encoding.UTF8;

Assert.True(encoding.GetPreamble().Length > 0);
WriteTwoEventsAndCheckOutputFileLength(maxBytes, encoding);
}

[Fact]
public void WhenLimitIsSpecifiedAndEncodingHasNoPreambleDataIsCorrectlyAppendedToFileSink()
{
long? maxBytes = 5000;
var encoding = new UTF8Encoding(false);

Assert.Equal(0, encoding.GetPreamble().Length);
WriteTwoEventsAndCheckOutputFileLength(maxBytes, encoding);
}

[Fact]
public void WhenLimitIsNotSpecifiedAndEncodingHasNoPreambleDataIsCorrectlyAppendedToFileSink()
{
long? maxBytes = null;
var encoding = new UTF8Encoding(false);

Assert.Equal(0, encoding.GetPreamble().Length);
WriteTwoEventsAndCheckOutputFileLength(maxBytes, encoding);
}

private static void WriteTwoEventsAndCheckOutputFileLength(long? maxBytes, Encoding encoding)
{
using (var tmp = TempFolder.ForCaller())
{
var path = tmp.AllocateFilename("txt");
var evt = Some.LogEvent("Irelevant as it will be replaced by the formatter ");
var actualEventOutput = "x";
var formatter = new FixedOutputFormatter(actualEventOutput);
var eventOuputLength = encoding.GetByteCount(actualEventOutput);

using (var sink = new FileSink(path, formatter, maxBytes, encoding: encoding))
{
sink.Emit(evt);
}
var size = new FileInfo(path).Length;
Assert.Equal(encoding.GetPreamble().Length + eventOuputLength, size);

//write a second event to the same file
using (var sink = new FileSink(path, formatter, maxBytes, encoding: encoding))
{
sink.Emit(evt);
}

size = new FileInfo(path).Length;
Assert.Equal(encoding.GetPreamble().Length + eventOuputLength * 2, size);
}
}
}
}

25 changes: 25 additions & 0 deletions test/Serilog.Sinks.File.Tests/FixedOutputFormatter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
using Serilog.Formatting;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convention in the test project is for these to go into the Support namespace/folder.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Serilog.Events;
using System.IO;

namespace Serilog.Tests
{
public class FixedOutputFormatter : ITextFormatter
{
string _substitutionText;

public FixedOutputFormatter(string substitutionText)
{
_substitutionText = substitutionText;
}

public void Format(LogEvent logEvent, TextWriter output)
{
output.Write(_substitutionText);
}
}
}