Should a Factory ever be generated per request?

Question

I'm working with AutoFac to do some DI. I think I've got a decent grip on things, but just ran into a question I had with my code and thought I'd check:

public class AutofacRegistrations
{
    public static void RegisterAndSetResolver()
    {
        // Create the container builder.
        var containerBuilder = new ContainerBuilder();

        // Register the Web API controllers.
        containerBuilder.RegisterApiControllers(Assembly.GetExecutingAssembly());

        //  Only generate one SessionFactory ever because it is expensive.
        containerBuilder.Register(x => new NHibernateConfiguration().Configure().BuildSessionFactory()).SingleInstance();

        //  Everything else wants an instance of Session per HTTP request, so indicate that:
        containerBuilder.Register(x => x.Resolve<ISessionFactory>().OpenSession()).InstancePerApiRequest();
        containerBuilder.Register(x => LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType)).InstancePerApiRequest();

        //  TODO: I don't THINK I should actually be making a DAO factory per API request. I think the whole point of a Factory is to make them once.
        containerBuilder.RegisterType<NHibernateDaoFactory>().As<IDaoFactory>().InstancePerApiRequest();
        containerBuilder.RegisterType<StreamusManagerFactory>().As<IManagerFactory>().InstancePerApiRequest();

        // Build the container.
        ILifetimeScope container = containerBuilder.Build();

        // Create the depenedency resolver.
        var dependencyResolver = new AutofacWebApiDependencyResolver(container);

        // Configure Web API with the dependency resolver.
        GlobalConfiguration.Configuration.DependencyResolver = dependencyResolver;
    }
}

You can see that I am registering an NHibernateDaoFactory and a StreamusManagerFactory as InstancePerApiRequest. This doesn't feel right to me because I think creating a Factory is supposed to be a relatively expensive endeavour and they should probably be .SingleInstance() similiar to the ISessionFactory.

However, my NHibernateDaoFactory has a dependency on Session:

...

public NHibernateDaoFactory(ISession session)
{
    if (session == null) throw new NullReferenceException("session");

    Session = session;
}

public IErrorDao GetErrorDao()
{
    return ErrorDao ?? (ErrorDao = new ErrorDao(Session));
}

....

and my ManagerFactory has a dependency on the DaoFactory:

...

public StreamusManagerFactory(ILog logger, IDaoFactory daoFactory)
{
    if (logger == null) throw new NullReferenceException("logger");
    if (daoFactory == null) throw new NullReferenceException("daoFactory");

    Logger = logger;
    DaoFactory = daoFactory;
}

public IErrorManager GetErrorManager()
{
    return ErrorManager ?? (ErrorManager = new ErrorManager(Logger, DaoFactory.GetErrorDao()));
}

...

This leaves me having to create new factories per request. Is this correct?

Answer 1

Just got one thing to say on your code:

Your Comments add close to no value!

your code is littered with comments and empty lines

//Create the Container Builder
var containerBuilder = new ContainerBuilder();

The comment is in fact almost exactly the same as the code below. And it's not only there, it's the same with every comment in the AutofacRegistrations you posted.

---

Other than that, if your factory has a dependency on Session you maybe did something wrong. Instead you could try to pass the Session to the already instantiated singleton factory, when calling the GetErrorDao() Function.

That would instead look something like that:

public IErrorDao GetErrorDao(Session session)
{
     return ErrorDao ?? (ErrorDao = new ErrorDao(session));
}

this would decouple both your NHibernateDaoFactory and your ManagerFactory and let them be free to use a singleton pattern instead. Which in turn greatly reduces the stress your application puts on the system.

You'd need to cascade this up the Dependency Chain, but that's a small price to pay to have a decoupled Factory Instance for all Requests / Sessions instead of one for each Session.
You'd thus need to modify your GetErrorManager too:

public IErrorManager GetErrorManager(Session session)
{
    return ErrorManager ?? (ErrorManager = new ErrorManager(Logger, DaoFactory.GetErrorDao(session)));
}