Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use injected repository service instead of static singleton #1453

Open
momvart opened this issue Dec 2, 2021 · 8 comments
Open

Use injected repository service instead of static singleton #1453

momvart opened this issue Dec 2, 2021 · 8 comments
Milestone

Comments

@momvart
Copy link

momvart commented Dec 2, 2021

Is your feature request related to a problem? Please describe.
I observed this problem when added quartz to my ASP.NET project and integration tests started to fail because of the error "Scheduler with the name xxx already exists". After investigations, I found out that this is because of a shared context between different instances of WebApplicationFactory. Reading the source codes led me to this line which shows a static repository is being used as a singleton. As the tests run in shared memory space, this static object is also shared between them.

Although the error could be solved by defining locks in custom hosted services, this behavior is breaking the isolation of tests and requires additional considerations in any case. For example, we cannot use separate databases for each test if they are going to run in parallel.

Describe the solution you'd like
Register the repository as a singleton service and inject it into the dependent classes.

Also relates to #399.

@lahma
Copy link
Member

lahma commented Dec 4, 2021

I think this requires some breaking changes in order to work. Unless you can figure out a way to support in 3.x without breaking API and compatibility? Scheduling for v4 for now.

@lahma lahma added this to the 4.0 milestone Dec 4, 2021
@kolpav
Copy link
Contributor

kolpav commented Mar 18, 2022

@lahma Is this problem still present on latest dev version? I added quartz into my integration tests using WebApplicationFactory with same problem described above its bummer quartz is not working well with it as the job scheduling is what I was looking the most to test. Is there some workaround for now? Knowing if this is still on your radar would be enough so I don't spend more time investigating well known problem. Other than that using quartz is a pleasure 🙏

@lahma
Copy link
Member

lahma commented Mar 18, 2022

Probably is still present in the latest dev version. A lot of changes are needed to make Quartz use DI services instead of current configuration scheme (and still support the old one to a degree).

In tests you could manually try to remove scheduler from scheduler repository or assign unique name (GUID or something) for each created scheduler.

@kolpav
Copy link
Contributor

kolpav commented Mar 18, 2022

@lahma Exactly what I am trying right now, thank you for quick response.

@rogierpennink
Copy link

rogierpennink commented Aug 10, 2022

@lahma Exactly what I am trying right now, thank you for quick response.

I'm not sure if you ever got it to work. I ran into exactly the same issue: I'm trying to run multiple integration tests for jobs in parallel using WebApplicationFactory.

The solution that ended up working for me was instantiating a new WebApplicationFactory instance per test, along with an integration-test specific path for the quartz configuration in my Startup.cs file that sets a unique scheduler id/name, basically guaranteeing that each test gets its own scheduler instance:

services.AddQuartz(q =>
{
    q.UseInMemoryStore();
    q.UseMicrosoftDependencyInjectionJobFactory();

    if (Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT") == "IntegrationTest")
    {
        q.SchedulerId = Guid.NewGuid().ToString();
        q.SchedulerName = Guid.NewGuid().ToString();
    }
    
    // Rest of config...
});

@kolpav
Copy link
Contributor

kolpav commented Aug 11, 2022

@lahma Hello is this going to be fixed in next 3. version? I am not sure if #1588 was eventually merged it seems there were conflicts and author is not responding.

@lahma
Copy link
Member

lahma commented Aug 11, 2022

@kolpav it's in v3 branch and available on MyGet already, but not in v4 (main).

@lahma
Copy link
Member

lahma commented Aug 11, 2022

And I mean thread-safety, not injection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants