adrg / go-wkhtmltopdf

Handcrafted Go bindings for wkhtmltopdf and high-level HTML to PDF conversion interface
https://pkg.go.dev/github.com/adrg/go-wkhtmltopdf
MIT License
249 stars 21 forks source link

Converting in gofuncs failed #3

Closed shaikemarc closed 4 years ago

shaikemarc commented 5 years ago

I tried to spawn x gofuncs, each created a converter but failed on access violation. Is this library safe for a concurrency use?

Thanks

adrg commented 5 years ago

@shaikemarc The package itself should be thread safe. However, wkhtmltox does not seem to support multithreading. Relevant links:

https://github.com/wkhtmltopdf/wkhtmltopdf/issues/1711 https://forum.dlang.org/thread/qmnmirbwmrvwexaewmtw@forum.dlang.org

I will try to find a solution to this issue, as running multiple converters concurrently is a common use case.

shaikemarc commented 5 years ago

@adrg thanks for that!

leandrosilva commented 4 years ago

Please, also refer to:

https://github.com/leandrosilva/go-wkhtmltopdf/blob/master/examples/ex2

Thanks!

adrg commented 4 years ago

@leandrosilva @shaikemarc I got a bit of free time and I thought I should give this issue a go. I managed to get it to work by running the conversion on the main thread. I added a Web page to PDF conversion server example to showcase this.

Would you be able to try the example and let me know if it works for you as well?

leandrosilva commented 4 years ago

As per my tests, yes, it has worked. Including the case with "window status".

I've been playing with wkhtmltox in other languages for awhile now (C++/Rust/Python/C#) and faced this "main thread" kind of problem many times until finally understand what happens.

This thread covers this topic quit well: https://forum.qt.io/topic/84485/qapplication-not-created-in-main-thread/9

Thanks for sharing your code.

leandrosilva commented 4 years ago

I added runtime.LockOSThread to the example I wrote using my original HTTP Handler (pdf.ConvertPostHandler) and it worked a charm.

https://github.com/leandrosilva/go-wkhtmltopdf/blob/master/examples/use-of-http-handler/main.go

Many thanks. I didn't know this method.

adrg commented 4 years ago

@leandrosilva That's great, I'm glad it worked for you as well. That means this issue can be closed.

adrg commented 4 years ago

@leandrosilva I added a couple of improvement in PR #6, including JSON/YAML marshal support for the conversion options. That should make it easier to parse HTTP conversion requests. Also added a Configurable web page to PDF conversion server example.

leandrosilva commented 4 years ago

That's cool. I've done sort of that with ConvertOptions. But that code is only on my branch and not in yours. I'll refactor that at some point and converge to the same master as of yours. If you have time, take a look at what I did with handler.go and use-of-http-handler example, perhaps you would like to include these to your master branch or something (with some refactoring, now we have ConverterOpts). Cheers.

leandrosilva commented 4 years ago

BTW, not sure whether it happens to you but I got "could not set converter option outlineDepth to 4" everytime. It only works if I set it to 0. Anything different of that fails.

leandrosilva commented 4 years ago

I just did that: https://github.com/adrg/go-wkhtmltopdf/pull/7 Cheers

adrg commented 4 years ago

@leandrosilva Hm, I don't have the outlineDepth issue. What version of wkhtmltopdf are you using, and on what OS? Perhaps the default should be 0 if different versions behave differently. I'm using version 0.12.6, downloaded from here.

leandrosilva commented 4 years ago

@adrg I'm using wkhtmltopdf 0.12.4 (with patched qt) on Windows, due to my job. We might default it to 0, to work around that issue, but I know the documentation for 0.12.6 says its default is 4. I guess we can just leave it as it is.

adrg commented 4 years ago

From what I can tell, the default has always been 4. The problem with 0.12.4 is when you set that property, right? If you don't set it at all, everything works ok? If that's the case, if we have the default be 0 in the package, that would result in the outlineDepth property not being set, and it would default to 4 in versions that support this property.

adrg commented 4 years ago

@leandrosilva I created a Discord channel so that issues like this one can be discussed in depth, or where users of the package can get support, etc. I added an invite link in README.md. It would be great to have you there.