MarcosMeli / FileHelpers

The FileHelpers are a free and easy to use .NET library to read/write data from fixed length or delimited records in files, strings or streams
https://www.filehelpers.net
MIT License
1.13k stars 349 forks source link

INotifyWrite is not called when using asynchronous methods #391

Closed bcallaghan-et closed 3 years ago

bcallaghan-et commented 3 years ago

I have several record types that I am using with the MultiRecordEngine which implement the INotifyWrite interface. When I write a file using the WriteFile method, the BeforeWrite and AfterWrite methods are called as expected. When I write a file using the BeginWriteFile and WriteNext methods, the before and after hooks are not called at all. Examining the source code shows that the internal property MustNotifyWrite is not checked during the asynchronous WriteRecord method, while it is checked in the synchronous WriteStream method. This is clearly an oversight, as the asynchronous ReadRecord checks the MustNotifyRead property and functions the same as the synchronous ReadStream method. Please fix this so the before/after write hooks are called when using the asynchronous style methods.

mcavigelli commented 3 years ago

Thank you for your comment, I will look into the issue next week.

In the meantime: Is there a reason why you are not using the method WriteStream(TextWriter writer, IEnumerable records) on the MulitRecordEngine? It might offer the expected behaviour and also writes in a streaming mode, expecting records as IEnumerable.

Matthias

bcallaghan-et commented 3 years ago

In this case, it was possible to use the WriteFile(string file, IEnumerable records) method instead of the BeginWriteFile(string file) method, and that is the workaround I applied. This particular file has a header, one or more detail records, and a trailer. The header and trailer are not static (otherwise I would have used the HeaderText and FooterText properties). When I first wrote the app, it seemed more intuitive to write the header, write the body, and then write the footer, rather than building up the list of records in memory and writing it to the file in one batch. I just happened to have an INotifyWrite implementation that wasn't being called, which is how I discovered this.

phumphreys commented 3 years ago

In my case, the data I have describes a hierarchical layout with some caveats. I build the hierarchy while reading. For writing, I use the WriteStream process and pass the engine down the hierarchy. This aspect is working quite well. However, there are fields on some records that can be formatted in multiple ways. Another field in the same class defines how it should be formatted. How I was hoping to solve the problem is to have concrete types that are hidden and use a string field to hold the parsed value. I could then inherit from INotifyRead and INotifyWrite and use the AfterRead to interrogate the definition field and then perform the conversion and set the concrete variable. I would then use the BeforeWrite to perform the conversion in the opposite direction.

In my investigation, it seems as though the RecordInfo variable always seems to reference the first typeof() sent into the constructor and never gets updated to reference the current record type being worked on. The result is the MustNotifyRead/MustNotifyWrite is checking the wrong object. So my AfterRead and BeforeWrite will never be called. Looking deeper into the code, it appears as though even if the above aspect was working correctly, the WriteRecord is missing MustNotifyWrite checks blocks for before/after the RecordToString call. I just started using this repo last week, so I could be missing a/some procedural steps. Thanks!

Paul

mcavigelli commented 3 years ago

I think there are two bugs hidden

1) It seems that the logic for writing is duplicated: File MultiRecordEngine, line 392 and line 879. The first occurrence publishes the events, the latter not. The bug should be fixed by removing the (incorrect) duplication.

2) Also to decide if the events should be fired or not does take into account that the recordinfo changes, as @phumphreys stated. One of the pull requests addresses that problem for reading: https://github.com/MarcosMeli/FileHelpers/pull/256/commits/cdfe2e5c666d9131b9f62674bcb3b6047b788fa1 But it does not consider, wether there is an event subscription on BeforeWriteRecord or AfterWriteRecord.

Thank you for your analysis. Unfortunately I cannot fix these bugs in April. Matthias

mcavigelli commented 3 years ago
mcavigelli commented 3 years ago

Thank you for your patience. These bugs should be fixed.

I will publish a release next week. Thank you for your report