patterncsharpMinor
Asynchronous task runner
Viewed 0 times
asynchronousrunnertask
Problem
This is my personal project. This class is responsible for running jobs asynchronously that are registered using dependency injection.
All improvement suggestions are welcome. It can be as small as using a better name for a variable, re-architect the whole class, whatever. I'm just trying to improve myself.
GitHub
```
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using Ninject;
using NLog;
using NzbDrone.Core.Model.Notification;
using NzbDrone.Core.Repository;
using PetaPoco;
namespace NzbDrone.Core.Providers.Jobs
{
///
/// Provides a background task runner, tasks could be queue either by the scheduler using QueueScheduled()
/// or by explicitly calling QueueJob(type,int)
///
public class JobProvider
{
private static readonly Logger Logger = LogManager.GetCurrentClassLogger();
private readonly IDatabase _database;
private readonly NotificationProvider _notificationProvider;
private readonly IList _jobs;
private static readonly object ExecutionLock = new object();
private Thread _jobThread;
private static bool _isRunning;
private static readonly List> _queue = new List>();
private ProgressNotification _notification;
[Inject]
public JobProvider(IDatabase database, NotificationProvider notificationProvider, IList jobs)
{
_database = database;
_notificationProvider = notificationProvider;
_jobs = jobs;
}
///
/// Initializes a new instance of the class. by AutoMoq
///
/// Should only be used by AutoMoq
[EditorBrowsable(EditorBrowsableState.Never)]
public JobProvider() { }
///
/// Gets the active queue.
///
public static List> Queue
{
get
{
return _queue;
}
All improvement suggestions are welcome. It can be as small as using a better name for a variable, re-architect the whole class, whatever. I'm just trying to improve myself.
GitHub
```
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using Ninject;
using NLog;
using NzbDrone.Core.Model.Notification;
using NzbDrone.Core.Repository;
using PetaPoco;
namespace NzbDrone.Core.Providers.Jobs
{
///
/// Provides a background task runner, tasks could be queue either by the scheduler using QueueScheduled()
/// or by explicitly calling QueueJob(type,int)
///
public class JobProvider
{
private static readonly Logger Logger = LogManager.GetCurrentClassLogger();
private readonly IDatabase _database;
private readonly NotificationProvider _notificationProvider;
private readonly IList _jobs;
private static readonly object ExecutionLock = new object();
private Thread _jobThread;
private static bool _isRunning;
private static readonly List> _queue = new List>();
private ProgressNotification _notification;
[Inject]
public JobProvider(IDatabase database, NotificationProvider notificationProvider, IList jobs)
{
_database = database;
_notificationProvider = notificationProvider;
_jobs = jobs;
}
///
/// Initializes a new instance of the class. by AutoMoq
///
/// Should only be used by AutoMoq
[EditorBrowsable(EditorBrowsableState.Never)]
public JobProvider() { }
///
/// Gets the active queue.
///
public static List> Queue
{
get
{
return _queue;
}
Solution
Taking the processqueue part.... I'd do something like :-
This means you only lock once ( you have a bug in that you don't lock when you remove), your try catch is finer grained, your using is finer grained.
But the I'd probably turn the tuple into a Class. The rest of the code has similar issues.
private void ExecuteJob(Tuple job)
{
try
{
Execute(job.Item1, job.Item2);
}
catch (Exception e)
{
using (NestedDiagnosticsContext.Push(Guid.NewGuid().ToString()))
{
Logger.FatalException("An error has occurred while processing queued job.", e);
}
}
}
private void ProcessQueue()
{
List> jobs;
lock (Queue)
{
jobs = Queue.ToList();
Queue.Clear();
}
jobs.ForEach(ExecuteJob);
Logger.Trace("Finished processing jobs in the queue.");
}This means you only lock once ( you have a bug in that you don't lock when you remove), your try catch is finer grained, your using is finer grained.
But the I'd probably turn the tuple into a Class. The rest of the code has similar issues.
Code Snippets
private void ExecuteJob(Tuple<Type,int> job)
{
try
{
Execute(job.Item1, job.Item2);
}
catch (Exception e)
{
using (NestedDiagnosticsContext.Push(Guid.NewGuid().ToString()))
{
Logger.FatalException("An error has occurred while processing queued job.", e);
}
}
}
private void ProcessQueue()
{
List<Tuple<Type, int>> jobs;
lock (Queue)
{
jobs = Queue.ToList();
Queue.Clear();
}
jobs.ForEach(ExecuteJob);
Logger.Trace("Finished processing jobs in the queue.");
}Context
StackExchange Code Review Q#3848, answer score: 2
Revisions (0)
No revisions yet.