HakanL / WkHtmlToPdf-DotNet

C# .NET Core wrapper for wkhtmltopdf library that uses Webkit engine to convert HTML pages to PDF.
GNU Lesser General Public License v3.0
366 stars 66 forks source link

With Multiple requests fails #124

Closed elreyponce closed 1 month ago

elreyponce commented 6 months ago

Using .NET API and sending multiple requests at the same time I got this error

imagen

HakanL commented 6 months ago

Do you have multiple instances of the SynchronizedConverter? You need to just have one, as a singleton.

elreyponce commented 6 months ago

I have only one instance injected as Singleton in my API, just like your example for web projects. Actually this only happens to me with Internet Information Service with more than 8 request at time.

HakanL commented 6 months ago

Sounds like there are multiple instances somehow. I can't see what the actual exception is, but if you can show it in English and post your code where you call the convert and wire up the instance then we can see if we spot anything.

elreyponce commented 6 months ago

This is the information from a DMP file which I extracted from my application pool's windows process. imagen

I've tried blocking the concurrency using SemaphoreSlim before the "convert" function but I had the same error.

Here's part of my code.

  1. This is how I declare IConverter as a singleton intance. imagen

  2. Injecting the IConverter interface. imagen

  3. This is how I invoke the PDF generation. imagen

HakanL commented 6 months ago

Your code looks correct and solid. It sounds like IIS (or .NET hosting, not sure) has a limit on how long the requests can take (and PDF generation takes a while) and when you have multiple concurrent requests then you'll be over the limit (the WkHtmlToPdf project only allows a single concurrent request, the more requests you have the longer each thread will wait). I typically have a queue for processing long-running jobs and not have the web server hang for the result.

elreyponce commented 6 months ago

Thank for your quick responses, and for your work with this library.

Is it difficult to change the "Convert" method to an asynchronous one? That is the suggestion I saw analyzing the file. imagen

HakanL commented 6 months ago

Probably not possible, this project is just a wrapper for the native library that does the actual work, and it's unfortunately only single-threaded. You could potentially load multiple app-domains and load the library in each one, but I've never done anything like this so I'm not sure how to do it, or if it would help, it's just a thought.

elreyponce commented 6 months ago

Ok, I'll try it. Thank you!!!

mwwhited commented 4 months ago

If you end up with multiple instances of the IOC container SynchronizedConverter will not be safe enough. Here is a fix that was able to ensure a real single instance even across multiple IServiceProvider instances

    private static IConverter? _converter; 

    public static IServiceCollection TryAddWkHtmlToPdfServices(this IServiceCollection services)
    {
        services.AddTransient<ITools, PdfTools>();
        // Another option is to change the internal fields in the SynchronizedConverter into static fields instead of instance fields
        services.AddSingleton<IConverter>(sp => _converter ??= ActivatorUtilities.CreateInstance<SynchronizedConverter>(sp));
        return services;
    }
HakanL commented 4 months ago

@mwwhited Hmm, why would you have multiple service providers, that sounds risky? But if that's the case then your code would work, the native library only supports single-threaded access, so it has to be a maximum of 1 library instance per AppDomain.

mwwhited commented 4 months ago

Primarily due to mstest and how it parallels applications since they are not process isolated. It would be possible is using middleware isolation within and application. Unlikely for this particular code but is possible. A better model for this framework would be an operating system level semaphore if you are saying there can only be one instance at a time.

Either way my IoC registration resolve the deadlock issue if anyone else is effected.

HakanL commented 4 months ago

I see, yeah for unit tests that can be an issue, but it shouldn't be in a "real" application. There can be multiple instances, just not in the same AppDomain (at least that's my understanding, I've never tested that ).

HakanL commented 1 month ago

I reviewed this again. It seems that you're creating a new task and then waiting on that. That's typically not a great way to do it, I believe you can get some deadlocks and issues. I would recommend that you just call it synchronously instead, adding it to a thread pool if necessary to run it in the background.

elreyponce commented 1 month ago

Hi @HakanL, thanks for your response. I did some tests with IIS and with Kestrel servers. With Kestrel the process never blocked when I sent many simultaneous requests. I had to do a redirection from the IIS server to the Kestrel server in order to solve it.

HakanL commented 1 month ago

@elreyponce I think IIS may spin up multiple processes somehow, which causes the issue. I know IIS does some special tricks in some scenarios.