Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  1. System.Threading.Timer already executes on a threadpool thread, and by calling Task.Factory.StartNew you are starting yet another thread which is wasteful. If you called Wait() on the task, the TPL could inline it and keep using the same thread. Another possibility is an approach such as this this (basically await Task.Delay instead of an actual timer), but that depends on whether you want the timer to fire exactly every n seconds (even if the previous iteration is still running, which is what you're doing now), or you want each iteration to start n seconds after the last one finished (which is usually what you want, and which you would get with the await Task.Delay approach).
  2. I don't exactly follow your cancellation code - you cancel if cancellation wasn't requested? Anyway, yes, calling Cancel multiple times is OK.
  3. Generally you should always use ThrowIfCancellationRequested when available.
  4. (a) Presumably if you start these tasks every n seconds where n isn't very large, then they are not long-running at all. I don't think that flag makes sense here. Read Toub's answer here. (b) Passing the state parameter is slightly more performant since you avoid the lambda capture, but that is probably premature optimization. Each iteration will get its "own" myjob, so don't worry about that either don't worry about that either. (c) These two methods are not exactly equivalent, specifically when the task itself returns a task (which is what you're doing). Read the part about Unwrap in Toub's article.

As for OperationCancelledException, you're seeing it and not TaskCancelledException because CancelltionToken.ThrowIfCancellationRequested, which is what you're using to cancel your tasks, throws OperationCanceledException. Apparently that makes Task.WhenAll throw OperationCanceledException and so on. But you shouldn't be worried about TaskCancelledException vs OperationCancelledException. The former inherits from the latter, so just catch the latter and be done with it.

One more thing, in your Task.Delay, I assume you meant to multiply by 1000 (not 100) to convert from milliseconds to seconds. It is then not clear what you attempt to achieve because the minimum is 15 seconds which is more than 4, so the cancellation code will always get called.

  1. System.Threading.Timer already executes on a threadpool thread, and by calling Task.Factory.StartNew you are starting yet another thread which is wasteful. If you called Wait() on the task, the TPL could inline it and keep using the same thread. Another possibility is an approach such as this (basically await Task.Delay instead of an actual timer), but that depends on whether you want the timer to fire exactly every n seconds (even if the previous iteration is still running, which is what you're doing now), or you want each iteration to start n seconds after the last one finished (which is usually what you want, and which you would get with the await Task.Delay approach).
  2. I don't exactly follow your cancellation code - you cancel if cancellation wasn't requested? Anyway, yes, calling Cancel multiple times is OK.
  3. Generally you should always use ThrowIfCancellationRequested when available.
  4. (a) Presumably if you start these tasks every n seconds where n isn't very large, then they are not long-running at all. I don't think that flag makes sense here. Read Toub's answer here. (b) Passing the state parameter is slightly more performant since you avoid the lambda capture, but that is probably premature optimization. Each iteration will get its "own" myjob, so don't worry about that either. (c) These two methods are not exactly equivalent, specifically when the task itself returns a task (which is what you're doing). Read the part about Unwrap in Toub's article.

As for OperationCancelledException, you're seeing it and not TaskCancelledException because CancelltionToken.ThrowIfCancellationRequested, which is what you're using to cancel your tasks, throws OperationCanceledException. Apparently that makes Task.WhenAll throw OperationCanceledException and so on. But you shouldn't be worried about TaskCancelledException vs OperationCancelledException. The former inherits from the latter, so just catch the latter and be done with it.

One more thing, in your Task.Delay, I assume you meant to multiply by 1000 (not 100) to convert from milliseconds to seconds. It is then not clear what you attempt to achieve because the minimum is 15 seconds which is more than 4, so the cancellation code will always get called.

  1. System.Threading.Timer already executes on a threadpool thread, and by calling Task.Factory.StartNew you are starting yet another thread which is wasteful. If you called Wait() on the task, the TPL could inline it and keep using the same thread. Another possibility is an approach such as this (basically await Task.Delay instead of an actual timer), but that depends on whether you want the timer to fire exactly every n seconds (even if the previous iteration is still running, which is what you're doing now), or you want each iteration to start n seconds after the last one finished (which is usually what you want, and which you would get with the await Task.Delay approach).
  2. I don't exactly follow your cancellation code - you cancel if cancellation wasn't requested? Anyway, yes, calling Cancel multiple times is OK.
  3. Generally you should always use ThrowIfCancellationRequested when available.
  4. (a) Presumably if you start these tasks every n seconds where n isn't very large, then they are not long-running at all. I don't think that flag makes sense here. Read Toub's answer here. (b) Passing the state parameter is slightly more performant since you avoid the lambda capture, but that is probably premature optimization. Each iteration will get its "own" myjob, so don't worry about that either. (c) These two methods are not exactly equivalent, specifically when the task itself returns a task (which is what you're doing). Read the part about Unwrap in Toub's article.

As for OperationCancelledException, you're seeing it and not TaskCancelledException because CancelltionToken.ThrowIfCancellationRequested, which is what you're using to cancel your tasks, throws OperationCanceledException. Apparently that makes Task.WhenAll throw OperationCanceledException and so on. But you shouldn't be worried about TaskCancelledException vs OperationCancelledException. The former inherits from the latter, so just catch the latter and be done with it.

One more thing, in your Task.Delay, I assume you meant to multiply by 1000 (not 100) to convert from milliseconds to seconds. It is then not clear what you attempt to achieve because the minimum is 15 seconds which is more than 4, so the cancellation code will always get called.

fixed some typos , embedded links
Source Link
forsvarir
  • 11.8k
  • 7
  • 39
  • 72
  1. System.Threading.Timer already executes on a threadpool thread, and by calling Task.Factory.StartNew you are starting yet another thread which is wasteful. If you called Wait() on the task, the TPL could inline it and keep singusing the same thread. Another possibility is an approach such as this (basically await Task.Delay instead of an actual timer), but that depends on whether you want the timer to fire exactly every n seconds (even if the previous iteration is still running, which is what you're doing now), or you want each iteration to start n seconds after the last one finished (which is usually what you want, and which you would get with the await Task.Delay approach).
  2. I don't exactly follow your cancellation code - you cancel if canelationcancellation wasn't requested? Anyway, yes, calling Cancel multiple timetimes is OK.
  3. Generally you should always use ThrowIfCancellationRequested when available: https://msdn.microsoft.com/en-us/library/dd997396(v=vs.110).aspxwhen available.
  4. (a) Presumably if you start these tasks every n seconds where n isn't very large, then they are not long-running at all. I don't think that flag makes sense here. Read Toub's answer here: https://social.msdn.microsoft.com/Forums/vstudio/en-US/8304b44f-0480-488c-93a4-ec419327183b/when-should-a-taks-be-considered-longrunning?forum=parallelextensionshere . (b) Passing the state parameter is slightly more performant since you avoid the lambda capture, but that is probably premature optimization. Each iteration will get its "own" myjob, so don't worry about that either: http://stackoverflow.com/a/271447/67824don't worry about that either . (c) These two methods are not exactly equivalent, specifically when the task itself returns a task (which is what you're doing). Read the part about Unwrap in Toub's article.

As for OperationCancelledException, you're seeing it and not TaskCancelledException because CancelltionToken.ThrowIfCancellationRequested, which is what you're using to cancel your tasks, throws OperationCanceledException. Apparently that makes Task.WhenAll throw OperationCanceledException and so on. But you shouldn't be worried about TaskCancelledException vs OperationCancelledException. The former inherits from the latter, so just catch the latter and be done with it.

One more thing, in your Task.Delay, I assume you meant to multiply by 1000 (not 100) to convert from milliseconds to seconds. It is then not clear what you attempt to achieve because the minimum is 15 seconds which is more than 4, so the cancellation code will always get called.

  1. System.Threading.Timer already executes on a threadpool thread, and by calling Task.Factory.StartNew you are starting yet another thread which is wasteful. If you called Wait() on the task, the TPL could inline it and keep sing the same thread. Another possibility is an approach such as this (basically await Task.Delay instead of an actual timer), but that depends on whether you want the timer to fire exactly every n seconds (even if the previous iteration is still running, which is what you're doing now), or you want each iteration to start n seconds after the last one finished (which is usually what you want, and which you would get with the await Task.Delay approach).
  2. I don't exactly follow your cancellation code - you cancel if canelation wasn't requested? Anyway, yes, calling Cancel multiple time is OK.
  3. Generally you should always use ThrowIfCancellationRequested when available: https://msdn.microsoft.com/en-us/library/dd997396(v=vs.110).aspx.
  4. (a) Presumably if you start these tasks every n seconds where n isn't very large, then they are not long-running at all. I don't think that flag makes sense here. Read Toub's answer here: https://social.msdn.microsoft.com/Forums/vstudio/en-US/8304b44f-0480-488c-93a4-ec419327183b/when-should-a-taks-be-considered-longrunning?forum=parallelextensions (b) Passing the state parameter is slightly more performant since you avoid the lambda capture, but that is probably premature optimization. Each iteration will get its "own" myjob, so don't worry about that either: http://stackoverflow.com/a/271447/67824 (c) These two methods are not exactly equivalent, specifically when the task itself returns a task (which is what you're doing). Read the part about Unwrap in Toub's article.

As for OperationCancelledException, you're seeing it and not TaskCancelledException because CancelltionToken.ThrowIfCancellationRequested, which is what you're using to cancel your tasks, throws OperationCanceledException. Apparently that makes Task.WhenAll throw OperationCanceledException and so on. But you shouldn't be worried about TaskCancelledException vs OperationCancelledException. The former inherits from the latter, so just catch the latter and be done with it.

One more thing, in your Task.Delay, I assume you meant to multiply by 1000 (not 100) to convert from milliseconds to seconds. It is then not clear what you attempt to achieve because the minimum is 15 seconds which is more than 4, so the cancellation code will always get called.

  1. System.Threading.Timer already executes on a threadpool thread, and by calling Task.Factory.StartNew you are starting yet another thread which is wasteful. If you called Wait() on the task, the TPL could inline it and keep using the same thread. Another possibility is an approach such as this (basically await Task.Delay instead of an actual timer), but that depends on whether you want the timer to fire exactly every n seconds (even if the previous iteration is still running, which is what you're doing now), or you want each iteration to start n seconds after the last one finished (which is usually what you want, and which you would get with the await Task.Delay approach).
  2. I don't exactly follow your cancellation code - you cancel if cancellation wasn't requested? Anyway, yes, calling Cancel multiple times is OK.
  3. Generally you should always use ThrowIfCancellationRequested when available.
  4. (a) Presumably if you start these tasks every n seconds where n isn't very large, then they are not long-running at all. I don't think that flag makes sense here. Read Toub's answer here . (b) Passing the state parameter is slightly more performant since you avoid the lambda capture, but that is probably premature optimization. Each iteration will get its "own" myjob, so don't worry about that either . (c) These two methods are not exactly equivalent, specifically when the task itself returns a task (which is what you're doing). Read the part about Unwrap in Toub's article.

As for OperationCancelledException, you're seeing it and not TaskCancelledException because CancelltionToken.ThrowIfCancellationRequested, which is what you're using to cancel your tasks, throws OperationCanceledException. Apparently that makes Task.WhenAll throw OperationCanceledException and so on. But you shouldn't be worried about TaskCancelledException vs OperationCancelledException. The former inherits from the latter, so just catch the latter and be done with it.

One more thing, in your Task.Delay, I assume you meant to multiply by 1000 (not 100) to convert from milliseconds to seconds. It is then not clear what you attempt to achieve because the minimum is 15 seconds which is more than 4, so the cancellation code will always get called.

Source Link
  1. System.Threading.Timer already executes on a threadpool thread, and by calling Task.Factory.StartNew you are starting yet another thread which is wasteful. If you called Wait() on the task, the TPL could inline it and keep sing the same thread. Another possibility is an approach such as this (basically await Task.Delay instead of an actual timer), but that depends on whether you want the timer to fire exactly every n seconds (even if the previous iteration is still running, which is what you're doing now), or you want each iteration to start n seconds after the last one finished (which is usually what you want, and which you would get with the await Task.Delay approach).
  2. I don't exactly follow your cancellation code - you cancel if canelation wasn't requested? Anyway, yes, calling Cancel multiple time is OK.
  3. Generally you should always use ThrowIfCancellationRequested when available: https://msdn.microsoft.com/en-us/library/dd997396(v=vs.110).aspx.
  4. (a) Presumably if you start these tasks every n seconds where n isn't very large, then they are not long-running at all. I don't think that flag makes sense here. Read Toub's answer here: https://social.msdn.microsoft.com/Forums/vstudio/en-US/8304b44f-0480-488c-93a4-ec419327183b/when-should-a-taks-be-considered-longrunning?forum=parallelextensions (b) Passing the state parameter is slightly more performant since you avoid the lambda capture, but that is probably premature optimization. Each iteration will get its "own" myjob, so don't worry about that either: http://stackoverflow.com/a/271447/67824 (c) These two methods are not exactly equivalent, specifically when the task itself returns a task (which is what you're doing). Read the part about Unwrap in Toub's article.

As for OperationCancelledException, you're seeing it and not TaskCancelledException because CancelltionToken.ThrowIfCancellationRequested, which is what you're using to cancel your tasks, throws OperationCanceledException. Apparently that makes Task.WhenAll throw OperationCanceledException and so on. But you shouldn't be worried about TaskCancelledException vs OperationCancelledException. The former inherits from the latter, so just catch the latter and be done with it.

One more thing, in your Task.Delay, I assume you meant to multiply by 1000 (not 100) to convert from milliseconds to seconds. It is then not clear what you attempt to achieve because the minimum is 15 seconds which is more than 4, so the cancellation code will always get called.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /