Closed JasonBock closed 5 years ago
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.
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.
Good initiative! Reminders about await
tasks would be could.
We wrote a number of analyzers - but mainly they targeted issues we found in existing code we that needed to port into orleans.
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
What analyzers would prove most useful for Orleans?
Are the following ideas still good?
Thanks
@Kavignon I think the top candidate should be flagging .Wait(0
and .Result
being called on unresolved Task
s (or maybe even any Task
). Those are thread blocking operations and should never be used inside grain code.
@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
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.
@sergeybykov Then my PR here could be to flag as an error whenever a developer uses .Wait
anywhere? Even outside of the grain scope?
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;
```
Well, in this situation, it could very much be the following analyzers:
?
For Result, it should be a warning?
As per https://github.com/aspnet/Security/issues/59, there should be no .Result
whatsoever.
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
@sergeybykov so what would be the correct way to implement that trivial timeout logic?
should it be replaced to var resulsts = work.GetAwaiter().GetResult();
?
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.
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.
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
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:
Task
or aTask<T>
.sealed
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?