Orleans: Analyzers for Orleans

[JasonBock]

I've been playing with Orleans for a while now, and I'd love to see some analyzers written for the framework so errors could be found as soon as you type them. Such as:

• Methods on grain interfaces should always return a Task or a Task<T>.
• Grains cannot be sealed
• Grain state classes must be serializable

At least I think these are some of the rules a developer must adhere to. There may be others. My point is, I'd like to see these analyzers for this framework. I'd also be interested in writing them as well. Are there any others that can be codified with analyzers?

[veikkoeeva]

Orleans being used in distributed systems, maybe checking datetimes are DateTime.UtcNow or something having the offset included.

<edit: In other words, have recommendations that are in general good especially for distributed systems. Sorry of the poor phrasing and delay of improving it. Also, ping @ReubenBond, who has started such an analyzer, I believe.

[ReubenBond]

I had a prototype of an analyzer for our [SerializerMethod] etc attributes, but I was waiting for the relevant tooling be buildable with .NET Standard/Core before submitting a PR. Maybe it's compatible now.

[galvesribeiro]

Good initiative! Reminders about await tasks would be could.

[amccool]

We wrote a number of analyzers - but mainly they targeted issues we found in existing code we that needed to port into orleans.

• https://github.com/amccool/AlexBellLSD - for finding ArrayList usage (bad on the serializers) from old school coders
• https://github.com/bstauff/FindStatics - for finding buried statics, locking syntax sugar

Having something along the lines of a Best Practices, as well as something to identify coding issues (as @JasonBock states) would be greatly welcomed.

Sadly our analyzers where more indicative of a poor coding techniques in the legacy code. Whether or not to include things we found is a judgement call probably based more on dealing with legacy code than green-field.

Starting a new project in https://github.com/OrleansContrib - then attaching issues to that project seems like the right beginnings

[Kavignon]

What analyzers would prove most useful for Orleans?

Are the following ideas still good?

• Grains can't be sealed?(GrainReferenceTypeRequireSealedAttributeAnalyzer)
• Methods must be asynchronous(GrainInterfaceMethodMustBeAsyncAnalyzer)

Thanks

[sergeybykov]

@Kavignon I think the top candidate should be flagging .Wait(0 and .Result being called on unresolved Tasks (or maybe even any Task). Those are thread blocking operations and should never be used inside grain code.

[Kavignon]

@sergeybykov How would I know when a task is unresolved? Because it wouldn't be to hard to look for .Wait invocation or the use of Result property

[sergeybykov]

That's why I thought that flagging all invocations of .Wait() and .Result is probably easier than to figure out is a Task is unresolved. .Wait() should never be used anyway.

[Kavignon]

@sergeybykov Then my PR here could be to flag as an error whenever a developer uses .Waitanywhere? Even outside of the grain scope?

[galvesribeiro]

The .Result is tricky...

There are legitimate cases like this:
```
var work = DoSomeWork(); // Intentionally the returning Task wasn't awaited here
var timeout = Task.Delay(10000);

var first = await Task.WhenAny(work, timeout);

if(first == timeout) throw new TimeoutException();

var results = work.Result;
```

[Kavignon]

Well, in this situation, it could very much be the following analyzers:

• Cannot Block Task Until Completed - CannotBlockCurrentThreadWithWaitInvocationAnalyzer (error) -
• Grains can't be sealed - GrainReferenceTypeDoNotRequireSealedAttributeAnalyzer (error)

?

[Kavignon]

For Result, it should be a warning?

[sergeybykov]

As per https://github.com/aspnet/Security/issues/59, there should be no .Result whatsoever.

[Kavignon]

Ok so, I’ll only make a pull request based on making the asynchrous workflow safer at compile time by removing usage of Wait & Result ! Sounds good? @sergeybykov

[galvesribeiro]

@sergeybykov so what would be the correct way to implement that trivial timeout logic?

should it be replaced to var resulsts = work.GetAwaiter().GetResult();?

[Kavignon]

If I recall the doc correctly, doing this that way is still blocking the
async workflow and waiting for the thread to give back the results of the
background operation.

[sergeybykov]

should it be replaced to var resulsts = work.GetAwaiter().GetResult();?

If you can't turn it to var result = await Work();, then yes. Although as @Kavignon mentioned, that might block the thread unless the Task is resolved.

[ReubenBond]

The first analyzer has been merged, thanks to @mholo65! #5589

I will close this issue and we can open new issues/PRs for specific analyzers that individuals would like to see