UDP и C# async/await

[mayorovp]

Замечания.
1. Достаточно было написать в основном методе

using (cancellationToken.Register(() => tcs.TrySetCancelled()))
    await tcs.Task;

Это куда проще, чем получившийся метод-расширение WithCancellation

2. Метод-расширение WithCancellation тоже пишется проще.

public static async Task<T> WithCancellation<T>(this Task<T> task, CancellationToken cancellationToken) {
    await task.ContinueWith(_ => {}, cancellationToken, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default);
    return await task;
}

3. У вас в случае отмены задачи остается мусор — а все потому что _tcsDictionary.Remove(key) надо делать в блоке finally!

4. По поводу самого цикла чтения — имейте в виду, этот цикл навсегда забирает один из потоков пула. Если таких циклов во всей программе случайно около 20 и более — начнутся загадочные проблемы. Вам следовало бы либо явно создать отдельный поток — либо использовать конструкцию await и тут тоже.

5. У вас словарь _tcsDictionary никак не защищен. Следует либо использовать специальный однопоточный планировщик задач (и добавить всюду проверки, что метод именно в этом потоке и выполняется) — либо заменить Dictionary на ConcurrentDictionary

[NeoNN]

Спасибо, ценные комментарии. По поводу 1, 2, и особенно 3 пункта — полностью согласен. Метод SendReceiveUdpAsync должен быть обернут в try{} finally{}, где удаляется значение словарика. По пункту 4 — цикл чтения всего один, а вот насчет 5го пункта я не совсем понимаю, какие проблемы могут возникнуть и в каких ситуациях, если будет использован простой Dictionary, а не Concurrent?

[mayorovp]

а вот насчет 5го пункта я не совсем понимаю, какие проблемы могут возникнуть и в каких ситуациях, если будет использован простой Dictionary, а не Concurrent?

А попробуйте — и увидите сами. Создайте 1000 потоков, которые будут сначала добавлять что-нибудь в словарь, а потом удалять обратно — и так много раз.

Надо просто запомнить — нельзя обращаться к одному и тому же объекту из разных потоков одновременно — если в документации явно не разрешено обратное.

[mayorovp]

Можно. Но ConcurrentDictionary работает быстрее, чем обычный Dictionary, обернутый в lock.

[NeoNN]

Так ведь метод вызывается из одного потока, а не из нескольких. Нужно ли здесь обеспечение кросспоточной безопасности, если оно все равно не понадобится (YAGNI)?

[mayorovp]

Я вижу как минимум два потока — в одном вызывается метод SendReceiveUdpAsync, а в другом крутится цикл чтения.

[NeoNN]

А и правда что. Даже несмотря на то, что в цикле чтения значения из словарика только считывается — все равно надо обеспечить безопасность.

[andreycha]

2. Метод WithCancellation написан Стивеном Таубом. Вы уверены, что знаете лучше него, как правильно? :)

[mayorovp]

Да, уверен. Если бы я писал комментарий к той статье, на которую вы дали ссылку — я бы написал то же самое :)

[andreycha]

Да, вы правы. Пролистав комментарии до конца, нашел предложение, практически идентичное по реализации:

public static Task<T> WithCancellation<T>(this Task<T> task, CancellationToken token)
{
    return task.ContinueWith(t => t.GetAwaiter().GetResult(), token);
}

Ответ автора:

That's reasonable. If you're planning to use this for real, you'd probably want a few tweaks:
1. First check task.IsCompleted, and if it's true, just return 'task' rather than doing a continuation.
2. Pass TaskScheduler.Default as the scheduler and TaskContinuationOptions.ExecuteSynchronously as the continuation options to the call to ContinueWith.

Итого в сухом остатке только проверка.

[mayorovp]

Еще два замечания.
6. Перед вызовом _tcs.Dictionary.Remove(key) нет никакого смысла проверять наличие этого ключа там. Во-первых, этот ключ там по логике программы всегда есть, а во-вторых Remove сама умеет делать эту проверку.

7. В цикле чтения вместо SetResult надо использовать TrySetResult, особенно если вы последовали моему совету номер 1. В противном случае, если успеть отменить чтение, когда ключ уже получен из словаря, но SetResult еще не вызван — весь цикл чтения упадет с ошибкой.

8. Если в цикле чтения возникнет ошибка — она будет проглочена, и никто о ней не узнает. Просто программа внезапно перестанет получать ответы от серверов, и все… Лучше бы эту ошибку как-нибудь логировать, а еще лучше — пытаться пересоздать сокет.

[mayorovp]

9. Вместо использования Tuple<string, int> в качестве ключа в словаре можно использовать непосредственно класс IPEndPoint. Это ничуть не усложнит код SendReceiveUdpAsync — но значительно упростит код цикла чтения.

10. В редких случаях вызов _client.Send(msg, msg.Length, ip, port) может заблокировать поток на некоторое время. Внутри асинхронных функций лучше бы использовать метод SendAsync.

[gandjustas]

Сделал вариант попроще на RX — habrahabr.ru/post/238445/

[NeoNN]

Здорово, уже прочитал! Стоит изучить Rx в свободное время.