Fix unsafe cast in ArrayConverter #5732
Conversation
| @@ -353,6 +314,58 @@ public Continuation(object handle, delegate*<Task, object, int[], void> continua | |||
|
|
|||
| public void Invoke(Task task, object collection, int[] indices) => _continuation(task, collection, indices); | |||
| } | |||
| } | |||
|
|
|||
| abstract class ArrayConverter<T> : PgStreamingConverter<T> where T : notnull | |||
There was a problem hiding this comment.
No more class constraint, should make ROM easier to implement too now
a08239a to
463e28f
Compare
| SetValue(collection, indices, new ValueTask<TElement>((Task<TElement>)task).Result); | ||
| // Justification: exact type Unsafe.As used to reduce generic duplication cost. | ||
| Debug.Assert(task is Task<TElement>); | ||
| // Using .Result on ValueTask is equivalent to GetAwaiter().GetResult(), this removes TaskAwaiter<T> rooting. |
There was a problem hiding this comment.
Not new in this PR, but I seem to remember that .Result causes AggregateException to be thrown on failure, wrapping the actual exceptionn (whereas GetAwaiter().GetResult() unwraps)... May be worth making sure.
There was a problem hiding this comment.
As I understand ValueTask doesn't do so due to the expectation that it's only used for sequential async flows (single consumer single producer). As opposed to Task supporting Dataflow, Task.WhenAll and friends, where exceptions from multiple tasks can come together in one.
| // We're using ValueTask.Result here to avoid rooting any TaskAwaiter or ValueTaskAwaiter types. | ||
| // On ValueTask calling .Result is equivalent to GetAwaiter().GetResult() w.r.t. exception wrapping. | ||
| return new ValueTask<T>(task: (Task<T>)task).Result!; | ||
| // Justification: exact type Unsafe.As used to reduce generic duplication cost. |
There was a problem hiding this comment.
Missed this assert and justification in #5554
| SetValue(collection, indices, new ValueTask<TElement>((Task<TElement>)task).Result); | ||
| // Justification: exact type Unsafe.As used to reduce generic duplication cost. | ||
| Debug.Assert(task is Task<TElement>); | ||
| // Using .Result on ValueTask is equivalent to GetAwaiter().GetResult(), this removes TaskAwaiter<T> rooting. |
There was a problem hiding this comment.
As I understand ValueTask doesn't do so due to the expectation that it's only used for sequential async flows (single consumer single producer). As opposed to Task supporting Dataflow, Task.WhenAll and friends, where exceptions from multiple tasks can come together in one.
(cherry picked from commit 81e2f6e)
|
Backported to 8.0.4 via 1312c64 |
Fixes #5715