Issue #23 workaround #24
Conversation
…ng an encoding with preamble and a file size limit
| @@ -60,7 +60,7 @@ public override long Position | |||
|
|
|||
| public override long Seek(long offset, SeekOrigin origin) | |||
| { | |||
| throw new NotSupportedException(); | |||
| return _stream.Seek(offset, origin); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
| @@ -0,0 +1,25 @@ | |||
| using Serilog.Formatting; | |||
There was a problem hiding this comment.
Convention in the test project is for these to go into the Support namespace/folder.
|
Hi Eamon, I will make these changes by hand on the way "through the works" - thanks again for the investigation and PR. |
|
Thanks Nic, I unplugged for the weekend so would have got to this today otherwise. |
|
Unplugging over the weekend is a good policy, I try to do the same :-) ... If you have a chance to spin up the 3.1.1 dev build it would be great to get the extra validation that all is working smoothly now. Cheers! |
|
just a few quick checks with Serilog.Sinks.File 3.1.1-dev-00754 and the issue appears solved with this version. |
|
Great, thanks. #25 will push this through to master once I've done some more testing on different configurations here. |
Tests to demonstrate the issue and a workaround for it. I wouldn't consider the workaround a final solution though.