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

INotifyWrite is not called when using asynchronous methods #391

Closed
bcallaghan-et opened this issue Apr 7, 2021 · 6 comments · Fixed by #401
Closed

INotifyWrite is not called when using asynchronous methods #391

bcallaghan-et opened this issue Apr 7, 2021 · 6 comments · Fixed by #401
Assignees
Projects

Comments

@bcallaghan-et
Copy link

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link

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
Copy link
Collaborator

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: cdfe2e5
    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 mcavigelli self-assigned this May 3, 2021
@mcavigelli mcavigelli added this to In progress in Release 3.5 May 3, 2021
@mcavigelli
Copy link
Collaborator

mcavigelli commented May 5, 2021

@mcavigelli
Copy link
Collaborator

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

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

@mcavigelli mcavigelli moved this from In progress to Done in Release 3.5 May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

3 participants