Csvhelper: CsvWriter.Flush() fails to call .Flush() on underlying TextWriter

[s3raph]

When writing to a MemoryStream I'm required to do the following

using (var stream = new MemoryStream())
{
    var w   = new StreamWriter(stream);
    var csv = new CsvWriter(sw);
    // Do stuff...
    csv.Flush();
    w.Flush();
}

My expectation would be that the Flush operation will be propagated through the underlying writers being utilized by the CsvWriter.

[JoshClose]

It depends on what you do. If you're calling csv.NextRecord() to end the line, it will flush the CsvWriter for you, then write a new line. You need to flush the StreamWriter yourself. If you want to have it happen automatically, set writer.AutoFlush = true. A writer will flush itself when it's buffer gets full. The reason CsvWriter doesn't do it is because if the case when you're writing to a slow source, like over the wire. You want it to go in one big chunk, and not every row.

[s3raph]

Thanks for the quick response @JoshClose :)

That certainly makes more sense now. I still feel that as a user of the library, the common assumption would be that CsvWriter would merely use the wrapped TextWriter's buffer, and provide access to those properties and methods. Having a method named Flush() definitely conveys the impression that the buffer will be flushed and the information transmitted to any underlying stream.

Would you consider accepting a pull request that implements Flush and FlushAsync on WritingContext that is then called only when Flush is called on the CsvWriter?

[JoshClose]

No, because it shouldn't flush on every row written.

[marcrocny]

Reopen Request

It's true that the underlying TextWriter should not be flushed on every line.

But by having Flush and FlushAsync methods on a class named *Writer which wraps an underlying TextWriter, you are setting up a semantic expectation that calling them will indeed flush everything that needs to be flushed. The intellisense "Serializes the row to the TextWriter." is not sufficient to override this expectation, given that there isn't a (non-esoteric) reason for lib consumer to flush to the TextWriter but not to the stream.

I too lost about a couple of hours to this issue. I am in a scenario where the stream itself is managed by an outside resource, so I could not close the writer. I was tripped up by incomplete files in testing.

Granted this is an established API, so renaming or even changing the functionality would be breaking. Could you change the Intellisense summary to

Serializes the row to the TextWriter, but does not flush the TextWriter.

This is certain to save other new users lost time.

[jrend]

I had the same expectation that csvWriter.Flush() would flush the stream.
csvWriter.Context.Writer.Flush() does actually flush the stream, I'm using that in a similar scenario with a memory stream -- and I really appreciate leaving this stuff as public, way too many over-encapsulated libraries out there :)
Not sure how much Flush() would be used from the outside, but would be nice to either have the public Flush() do an actual flush, and have the current Flush() become FlushInternal() which would do what Flush does right now.
Or the writer could get a new public FlushStream() that does this.Context.Writer.Flush() to encapsulate it a bit better.

[JoshClose]

I don't think I ever saw @marcrocny's post. I can understand the confusion and am open to suggestions on how to fix it. I'm OK with making breaking changes if it makes things more clear.

What about making CsvWriter.Flush protected? Can you think of any reason a person would actually want to call that?

[jrend]

In its current form I don't see a use for an external caller to call Flush(), unless they're messing with some internals (I haven't looked that much into it, don't know if there is some extensibility scenario that would take advantage of it).
That being said, this library seems quite popular, so just in case someone out there depends on Flush being public with the current behavior, maybe it's better to leave that as is to avoid the breaking change, and have a new method name for a new "flush the writers all the way to the stream" method?

[s3raph]

Glad that this one has been reopened.

I can only speak for myself, but feel that making Flush protected and creating a new method makes a good compromise. As a user, that then prompts me to go looking for the method I actually need rather than using Flush which I only assumed does what I need based on the name.

[marcrocny]

Along with an API-doc change (outlined above) I think making the API protected would be a good move.