New class loading safety

[splitice]

The most common failure for our unit tests is a DynamicExpresso bug. It's quite annoying too.

Every usage of DynamicExpresso is guarded by locks in our case however unit tests still fail. There appears to be a race condition in type loading, something that looks to require a global (app domain?) lock internally.

[xUnit.net 00:00:02.12]       DynamicExpresso.Exceptions.ParseException : No applicable constructor exists in type 'X4BFlow.Test.Nf9.TestTemplatePrototype+TemplateTest' (at index 55).
[xUnit.net 00:00:02.12]       Stack Trace:
[xUnit.net 00:00:02.12]            at DynamicExpresso.Parsing.Parser.ParseNew()

We can make this happen by running tests in xunit which occur in parallel between test fixtures. One interpreter per test case, however the loading of types on first use may still occur in parallel.

[metoule]

You're saying that a single run of your test works every time, but that it fails if it runs in parallel to another test? And that each test has its own instance of interpreter?

I find this surprising, because the only thing shared between instances of Interpreters are the primitive types. Custom types that are registered via Interpreter.Reference are scoped to their Interpreter instance. To resolve the constructor, the standard .NET API is used to get all the potential constructors from the type (via Type.GetConstructors()), and then candidates are filtered out.

It's also worth noting that if you see this error message, it means we successfully detected that you're trying to instantiate type X4BFlow.Test.Nf9.TestTemplatePrototype+TemplateTest, so the issue isn't type registration. The issue is that we can't find any constructor that matches the arguments provided.

Can you share an example of a test that fails? Maybe it's an issue with generics resolution.

[splitice]

@metoule Can do:

using System;
using System.Buffers.Binary;
using System.Collections.Generic;
using X4BFlow.Main.Nf9.Models;
using X4BFlow.Main.Nf9.NetflowParser;
using X4BFlow.Main.Nf9.NetflowParser.Attributes;
using X4BFlow.Main.Nf9.NetflowParser.FieldTypes;
using Xunit;

namespace X4BFlow.Test.Nf9
{
    public class TestTemplatePrototype: INetflowFlow
    {
        class TemplateTest: INetflowFlow
        {
            [NetflowFieldPropertySetter(FieldType.DIRECTION)]
            public byte u8;
            [NetflowFieldPropertySetter(FieldType.L4_SRC_PORT)]
            public UInt16 u16;

            [NetflowFieldPropertySetter(FieldType.CT_STATUS)]
            public UInt32 u32;
        }

        [Fact]
        public void TestUInt8()
        {
            TemplatePayloadPrototype payloadPrototype = new TemplatePayloadPrototype(typeof(TemplateTest), new NetflowParseRow[] { new NetflowParseRow(0, (ushort)FieldType.DIRECTION, 1) }, NetflowSession.Interpreter);

            Assert.Equal("new TemplateTest{u8=input[i]}", payloadPrototype.BuildFunctionBody());
            var fn = payloadPrototype.BuildTemplateFunc();
            Assert.NotNull(fn);
            var t = fn(new byte[] { 1 }, 0) as TemplateTest;
            Assert.Equal(1, t.u8);
        }

        [Fact]
        public void TestUInt16()
        {
            TemplatePayloadPrototype payloadPrototype = new TemplatePayloadPrototype(typeof(TemplateTest), new NetflowParseRow[] { new NetflowParseRow(0, (ushort)FieldType.L4_SRC_PORT, 2) }, NetflowSession.Interpreter);

            var fn = payloadPrototype.BuildTemplateFunc();
            Assert.NotNull(fn);
            byte[] input = new byte[2];
            BinaryPrimitives.TryWriteUInt16BigEndian(input, 1);
            var t = fn(new byte[] { 0, 1 }, 0) as TemplateTest;
            Assert.Equal(t.u16, 1);
        }

        [Fact]
        public void TestUInt32()
        {
            TemplatePayloadPrototype payloadPrototype = new TemplatePayloadPrototype(typeof(TemplateTest), new NetflowParseRow[] { new NetflowParseRow(0, (ushort)FieldType.CT_STATUS, 4) }, NetflowSession.Interpreter);

            var fn = payloadPrototype.BuildTemplateFunc();
            Assert.NotNull(fn);
            byte[] input = new byte[4];
            BinaryPrimitives.TryWriteUInt32BigEndian(input, 1);
            var t = fn(new byte[] { 0,0,0, 1 }, 0) as TemplateTest;
            Assert.Equal(1u, t.u32);
        }
    }
}

They share an interpreter, but all usage is controlled by a lock.

In this example there is only two interpreter usages:

public void BuildFunctionBody(ref Utf16ValueStringBuilder ret, Type targetType, HashSet<FieldType> alreadyConsumed)
    {
        lock (_interpreter)
        {
            if (_interpreter.ReferencedTypes.FirstOrDefault(a=>a.Type == targetType) == null)
            {
                _interpreter.Reference(targetType);
            }
        }
       // ...
}

    public Func<byte[], int, INetflowFlow> BuildTemplateFunc()
    {
        var body = BuildFunctionBody();
        Lambda l;
        lock (_interpreter)
        {
            l = _interpreter.Parse(body, ExpressionParams);
        }

        return l.Compile<Func<byte[], int, INetflowFlow>>();
    }

I'm well aware this is not a runnable example on its own. Creating a small test case that doesnt exercise our Netflow Template tooling is on my TODO list.

[splitice]

FYI I also tried locking the compile (although documentation suggests this is not necessary). This did not help.

These are the only functions called into Interpreter. lambda.Compile is called outside of the lock as is the Func<> returned.

[davideicardi]

I think that the problem is that you can have one thread that modify the Interpreter (calling Reference, ...) While another thread is calling Compile. This is not supported. For unit testing I suggest to not share the instances. For real usage code I suggest to have an initialization phase where you configure it, and then call only read only functions.

[splitice]

I think that the problem is that you can have one thread that modify the Interpreter (calling Reference, ...) While another thread is calling Compile. This is not supported.

This is not the case. As you can see the only 2 usages of interpreter are locked against the interpreter object.

These are the only 2 calls in the interpreter object.

I do plan to make a small runnable replication test at some point.

[splitice]

@davideicardi If you are trying to say that Lambda.Compile is not threadsafe (I've verified that placing this in a lock does not solve the problem...) then the README definately needs to be changed.

After all it is explicit in stating: Lambda and Parameter classes are completely thread safe.

[splitice]

Ok, I found the problem. For reference @davideicardi and @metoule.

When a type is referenced it's referenced by its Name, not it's FullName therefore when the types that do not exist in the interpreter are referenced one replaced the other because they share the same Name (but not FullName)

This is probably worth a documentation note against public Interpreter Reference(Type type) using the alternative method with a typeName parameter would have prevented this issue.

If this issue is viewed as sufficient documentation thats fine with me too, and it may be closed.

[metoule]

I don't think using the other Reference overload would have prevented your issue. The typeName argument is the identifier that is used by DynamicExpresso to detect a referenced typed in an expression. For example in a constructor call:

new TypeName()

the TypeName identifier is used as the key to map it to a referenced type. You can't reference two types with the same name because there would be no way to know which type to use based on that single identifier. For example, :

namespace N1 { public class TypeName {} }
namespace N2 { public class TypeName {} }

var expr = "new TypeName()"; // <-- no way to know which type to use

[metoule]

We could use the type's FullName for resolution, and raise an AmbiguousTypeException if there's an ambiguity, but that would require supporting FullNames during the identifier resolution, which is far from trivial:

var expr = "new N1.TypeName()"; // <-- this is not supported at the moment

[davideicardi]

I will close the issue, but if you need further assistence feel free to re-open it.

Just a clarification regarding thread safety: All the Interpreter functions that do not modify the instance can be called by multiple threads at the same time. But you cannot call Interpreter.Reference in one thread while calling Interpreter.Parse in another thread. This could result in unexpected errors. So technically Interpreter.Parse is thread safe, but only if used without changing the inner instance... I think this is quite common in .NET classes.