Member order from GetFields() causing errors

[bdemarzo]

I have an app that parses through a number of files, importing data from them using the FileHelpers library. As a result, I make multiple calls to code that looks like this:

FileHelperEngine<T> engine = new FileHelperEngine<T>();
T[] records = engine.ReadStream(reader);

If the code above is run multiple times with the same type (i.e. <T>), it'll cause errors after one or two runs.

Stepping through the code, I found the problem originating in the RecursiveGetFields() method in RecordInfo.cs. The problem, though, is not in FileHelpers; rather, it is in the fact that the .Net GetFields() method is not guaranteed to return members in any specific order.

A little research found an article (http://blogs.msdn.com/haibo_luo/archive/2006/07/09/661091.aspx) that mentions, among other things:

Quote:

As John mentioned in his post, every Reflection user should keep this in mind: our code should not count on the member order returned by GetFields, GetMethods and other similar GetXXXs calls. Given two fields (F1, F2) in one type, I think the compiler is free to emit F1 before F2, or vice versa. Also reflection does not guarantee to return fields in the order of how they present in the metadata; GetXXXs could be returning the members in different order at different running moments, which is illustrated by the following code. (Of course, they should have the same set of members)

My experience has shown that the first time GetFields() is called always returns items in the proper order; however, subsequent calls are not held to such requirements. Apparently, this is related to the framework's (ineffective) use of caching...

In trying to figure out the best solution to this, I can think of two possibilities:

Implement our own caching, which will store the fields retrieved by RecursiveGetFields() in our own cache, which will guarantee the same order. This would probably work in practice, since the first time reflection runs it returns members in order.

Add a new custom attribute which can be set to store the field order. We can then reorder the items retrieved by RecursiveGetFields() in the order needed. This is more reliable in that it ensures field order will be accurate even if reflection returns items in an arbitrary order. It'll also let you specify field order outside the order of your properties.

This is all meaningless if I'm missing something that has already been implemented to work around this... but if something hasn't, I'd like some thoughts before I work on a fix and submit a patch.

[bdemarzo]

OK, it seemed to be an easier fix than I thought! The below change adds simple caching of fields retrieved via reflection using a static hashtable. Worked fine in my situation, so I figured I'd share it here for discussion...

Subversion patch details follow.

Code:

Index: RecordInfo.cs =================================================================== --- RecordInfo.cs   (revision 602) +++ RecordInfo.cs   (working copy) @@ -27,6 +27,8 @@      /// <remarks>Is public to provide extensibility of DataSorage from outside the library.</remarks>      internal sealed class RecordInfo      { +      private static Hashtable mCachedFields = new Hashtable(); +          // --------------------------------------          // Constructor and Init Methods   @@ -51,6 +53,7 @@          private Regex mConditionRegEx = null;  #endif          internal int mFieldCount; +      internal bool mRecursiveClasses;            private ConstructorInfo mRecordConstructor;   @@ -150,9 +153,16 @@                // Create fields   -            ArrayList fields = new ArrayList(); -            RecursiveGetFields(fields, mRecordType, recordAttribute); +         ArrayList fields = null;   +         fields = mCachedFields[mRecordType] as ArrayList; +         if (fields == null) +         { +            fields = new ArrayList(); +            RecursiveGetFields(fields, mRecordType, recordAttribute); +            mCachedFields.Add(mRecordType, fields); +         } +              mFields = CreateCoreFields(fields, recordAttribute);              mFieldCount = mFields.Length;

[Marcos]

Brian

Thanks a lot again

Thats really weird I cant belive that .NET work in that way, not until I tried the example in the blog post and see what show me

Anyway I just finish to commit the changes that you made, at some point Matt and me talk about add caching at RunTime records level, and caching for a Type that direct return the RecordInfo instead of create all the fields again. This will give us faster warm up for ASP.NET and other apps that create engines a periodical intervals over the same type.

But with your solutiion we can solve this problem easier =)

Cheers, Im adding you at the top of the contributors list with us

See ya
_________________
Happy Coding
Marcos ( www.devoo.net )
If you like the library support us with some books or monetary donations
Vote FileHelpers in Codeproject or Bookmark us in Delicious or here, Thanks !

[Matt Campbell]

Perhaps I'm being a bit too literal, or too strict, in my interpretation of that quote, the original blog post from which it came, and the other posts linked from that post, but saying things like

Quote:

...code should not count on the member order returned by GetFields, GetMethods and other similar GetXXXs calls...

is an awful lot like saying "every time you've seen FileHelpers actually work, it was an accident."

If we are truly never supposed to be able to know for certain what the declared order of fields was in a class, then we have a FAR bigger problem than any amount of caching could ever solve. The examples I've seen so far seem to indicate that the GetFields() order tends to be "correct" (or at least constant) on the first call per application run (when the MemberInfo cache is empty). But how can FileHelpers know that the order was correct on its first call? We would essentially need to assume that the MemberInfo cache is still empty on the record class type - which also means that the calling application has not yet reflected on this type directly (which is unlikely anyway) or via any other Attribute-based libraries (which is unfortunately much more likely). Needing to declare FileHelpers incompatible with all other Attribute-based or Reflection-based code is not a pleasant thought. And then there's ASP.NET 2.0 - essentially, unless the GC runs between page hits, an ASP.NET 2.0 application using FileHelpers could suddenly stop working after a certain number of hits - or even worse, it might keep working but yield corrupted data due to reading compatible-typed fields out of order (for example - a record class of all Strings). [EDIT: it's late and I need sleep: I now see that caching RecordInfo should solve this last potential problem.]

Depending on the severity of this issue, we may need to provide some mechanism to users to explicitly specify field ordering. Another new Attribute maybe, like [FieldOrder(0)], [FieldOrder(1)], etc. Needing to sort an array at startup would bring a performance penalty though, so we would need some check to only sort when the order is explicitly specified like this.

But I think we need to start by writing a few Test Cases... (I can do this sometime tomorrow if nobody beats me to it.) They should be able to re-use any of the existing record classes in the Tests assembly. A simple foreach loop through a GetFields() call should suffice to test out-of-library reflection. We'll need tests which try doing this loop before creating the Engine, between creating 2 Engines, etc. And for the tests, the Engines don't need to actually read or write files, since the problem should be apparent just by Asserting that the names of the fields in the engine are in the same order as in the test case.

... and then of course, there's the problem that we can't even run more than one of these tests per testing session and expect valid results!

[Marcos]

Matt

At the moment the current solution provided by Brian works correctly in fact if you read the blog post the problem is based on a caching problem on .NET 2.0, in fact the fields order is important metadata that the caching is missing =(

Anyway the first call always return the valid order, yesterday I tried some NUnit tests and all work fine, in fact I use FileHelpers a lot in my work with more than 1.000.000 of records and I never get this errors, anyway I dont do ASP.NET apps so maybe the core of the problem can be there.

In the .NET 1.1 version there are no problems at all.

We need to review this anyway, the fact of add FieldOrder is a bit cumbersome and I dont like the idea =( but if there is no other way, we need to move forward that approach.

Cheers
_________________
Happy Coding
Marcos ( www.devoo.net )
If you like the library support us with some books or monetary donations
Vote FileHelpers in Codeproject or Bookmark us in Delicious or here, Thanks !

[Marcos]

YESSSSS =)

I made it work always !!! =)

The problem only appears if some code ask for the fields in different order, but near always, is very common to use GetFileds, using that method not cause any problem

Using the GetField method in the same order than the declaration not cause problems either, only when there is a call to GetField with some other field.

The GC.Collect(0) or GC.Collect() dont work always because if there is a reference to the cached fieldinfo the code get failing always.

changing the line 9 for this

FieldInfo f2 = typeof(C).GetField("F2"); // Line 9

Generates this results

F1 F2
F2 F1
F2 F1
F2 F1

After try some others solutions with the garbage collector, I give up and try another approach, this time I look with the debbuger the values of the field cache inside the RunTimeType and there I reset the cache with this simple code

Code:

object cache = mRecordType.GetType().GetProperty("Cache", BindingFlags.DeclaredOnly | BindingFlags.Instance | BindingFlags.NonPublic).GetValue(mRecordType, null); cache.GetType().GetField("m_fieldInfoCache", BindingFlags.FlattenHierarchy | BindingFlags.Instance | BindingFlags.NonPublic).SetValue(cache, null);

I will make the reference to of the first GetProperty and Getfile later static to avoid re request them

So far there is no security problems, we need to test it in an ASP.NET environment with some restriction, but as FileHelpers need reflection permissions I think that this code must work too.

Cheers
Thanks Brian to point this very important issue
_________________
Happy Coding
Marcos ( www.devoo.net )
If you like the library support us with some books or monetary donations
Vote FileHelpers in Codeproject or Bookmark us in Delicious or here, Thanks !

[bdemarzo]

Quote:

is an awful lot like saying "every time you've seen FileHelpers actually work, it was an accident."

This is the one thing that concerns me, too. An attribute-based approach could address this.

For example, take the following class:

Code:

public class Sample {     [FieldOrder(1)]     public string FieldOne;     [FieldOrder(2)]     public string FieldTwo;     [FieldOrder(3)]     public string FieldThree; }

One problem with this is you'd have to have some special rules to handle missing attributes, or require all properties to have the attribute. You'd have to handle duplicate values, too. All possible. You could even have attributes handle field order dynamically -- as in [FieldOrder(OrderDirection.After, "SomeFieldName")] ...

[Marcos]

[FieldOrder(1)] Attribute Implemented

Cheers
_________________
Happy Coding
Marcos ( www.devoo.net )
If you like the library support us with some books or monetary donations
Vote FileHelpers in Codeproject or Bookmark us in Delicious or here, Thanks !