-
Notifications
You must be signed in to change notification settings - Fork 76
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
InvalidProgramException while trying coyote on my library. #433
Comments
Thanks for reporting this @Kuinox! And sorry for the delay in responding, I was away last week and just saw your issue. Give me a few days to get on this & investigate, it might be some bug due to what you are showing me being an extension method and the rewriter might not be handling that properly. (At least this is my top hunch from what I see here.) |
Hi @Kuinox, I tried setup a similar mini test to see if I can repro this, but was not able to: public static class Extension
{
public static ValueTask<Task> PublishAsync(this Program @this, int value) => @this.PublishAsync(value);
}
public class Program
{
public ValueTask<Task> PublishAsync(int value) => ValueTask.FromResult((Task)Task.FromResult(value));
[Test]
public static async Task Test()
{
var program = new Program();
await program.PublishAsync(0);
}
} I run dotnet's ilverify in the end, which should catch these kind of bugs, and all passed:
What does ilverify give you if you run it from your side on your rewritten DLL? Are you able to send me a minimal snippet that exhibits this bug outside of your own repo? Perhaps similar to the one I wrote above, but that actually triggers the bug. Perhaps your code does something more intricate and its not just an extension method that looks like this. That will make it much easier for me to test it & debug without having to clone & build your repo. Thanks! |
I will try to do a minimal reproduction. |
Do you mind a minimal reproduction with one of my DLL as a dependency ? |
That is fine too, as long as you can give me instructions how to build and run it :) |
I made a repo here:
|
Hi @Kuinox, your repro was very helpful! Sorry took me a bit to get on this as it is year end and I am in the middle of a lot of other work, but I managed to dive into this issue and figure out what is going on. It is some very weird rewriting corner case that is not handled properly by the tool right now, but I have a workaround. Can you try changing: public static ValueTask<Task> PublishAsync(this MQTTClientAgent @this, ApplicationMessage message)
=> @this.PublishAsync(message.Topic, message.QoS, message.Retain, message.Payload); To simply use explicit brackets: public static ValueTask<Task> PublishAsync(this MQTTClientAgent @this, ApplicationMessage message)
{
return @this.PublishAsync(message.Topic, message.QoS, message.Retain, message.Payload);
} This should do the trick (at least it does for me) as it interestingly generates different IL. The IL generated in the simpler I will try fix the issue ASAP in the meantime (but as a lower priority since there is a workaround, hopefully in the next couple of releases), so leaving this issue open to track it. Also note that right now your coyote test will still not work properly even with this workaround in place. This has to do with the Hope this helps? Let me know if you need more help/clarifications! |
No worries, thanks for looking into this :D
I speculate it will only work in debug mode, since release mode compile to the same IL. Not a problem for me.
Yep, I managed to run tests, and it found a bug !
Bur it looks like it fails to reproduce the bug ?
|
Oh, I was not aware that this was the case with this pattern, thanks for the heads up, very helpful once I get to fix this issue properly!
This is awesome!
This is interesting, could you copy paste here the JSON configuration file that you are using for rewriting? Which DLLs are you rewriting? Is it only a partial set of the test dependencies? One very likely reason this might happen is due to the partially-controlled concurrency mode (enabled by default) that I mentioned earlier. Basically, to deterministically replay a bug using Could this be what is happening here? Do you see any logs mentioning "uncontrolled" if you enable |
Doesn't looks like:
It's available here: https://github.com/signature-opensource/CK-MQTT/blob/testing-coyote/Tests/CK.MQTT.Client.Tests/coyote.json I have put all our companies library as to be rewritten, and the behavior doesn't change. |
I retried by putting every single DLL in the "Assemblies" list(except the one on the exclude list), and it shows the same behavior |
That is strange! It must be some source of uncontrolled concurrency/data-nondeterminism that is not intercepted/logged by Coyote yet ... (if we figure what that is, I am happy to try support it, and at least intercept/log it first). About the deadlock do you have a sense yet what might be causing it? |
No, I need to investigate it. |
Hello,
I wanted to test out coyote to try to find bugs in my MQTT Library.
Sadly it looks like the IL rewrite has a problematic bug:
The corrupted method do something a bit unusual:
And the IL Diff:
If you need to reproduce it, I made a branch on my repo:
https://github.com/signature-opensource/CK-MQTT/tree/testing-coyote
The text was updated successfully, but these errors were encountered: