Fixes async / await usage

[jtkech]

Fixes #6919

[sebastienros]

Not the good fix. We should make the methods sync instead. Breaking change... Or at least another overload and we can change our own usages.

[jtkech]

@sebastienros Sorry i don't understand

E.g. _contentDefinitionManager.AlterTypeDefinition() has an alteration parameter that is an Action<> and then that can't be async, but this action delegate was using async / await resulting in non awaited tasks. In 2 other places, the delegate of an .AlterSomething() already uses .GetAwaiter().GetResult(), but not in 2 other places that i updated in this PR.

Maybe you didn't see that the above .AlterTypeDefinition() is called in the body of UpdateTypePartEditorAsync() that is async and that calls before e.g. await CreateContentShapeAsync(), and that the issue was the usage of async / await in the action delegate of .AlterTypeDefinition()

[deanmarcussen]

@jtkech maybe we should add extra extension methods to support a Func<T,Task> instead.

They are only extension methods so should be able to work as an overload without breaking anyone.

[jtkech]

@deanmarcussen

Yes, in an old PR i already suggested to make all async around content definition, including UpdateContentDefinitionRecord() that currently uses a .GetAwaiter().GetResult()

In fact, the 2 async / await usage we are fixing are not in the action delegate of .AlterTypeDefinition(), but in the action delegate of .WithPart() or WithField(), same issue but there are not extensions, but yes we could also create .WithPartAsync() and WithFieldAsync() builder extensions.

Maybe in another PR, here it just fixes an async / await usage in a delegate related to an Action<>, i already fixed the same kind of issue here a while ago, but we missed these two ones. Anyway, if nobody does it, i will merge it myself ;)

Hmm, @sebastienros when you said

Not the good fix. We should make the methods sync instead

Did you mean?

Not the good fix. We should make the methods async instead

Only one character changes the whole meaning, if so i now understand and i will go this way, sorry ;)

I didn't do that immediately as before you were not always a fan of making async things around content definition.

[jtkech]

Okay i made the change, so more things are async

But a little more changes, so @deanmarcussen if you can check that it still fixes your issue, if you have time ;)

As said, there are many other things that are not async around content definition, but at some point it was intentionally kept like this because the content definition record is cached, and to have a less overhead of awaited Task

Update: Okay just tried for the fun to follow your repro

With this PR it was working, after reverting it i could repro the same exception

[deanmarcussen]

@jtkech works for me, and I put a Task.Delay(1000) in on one of the queries, and could see it was still properly awaited before the session tried to dispose, so all good.

I'm assuming Seb meant async rather than sync as well, and while there's a little more code for us to maintain with both a normal version, and an async version, this is not code that needs to be maintained much, so I think it makes sense.

So all looks good to me, unless @sebastienros has meant something different :)

Yes, I understand about not making the content definitions themselves not async

[jtkech]

I merged it as it fixes an annoying issue