jung-kurt / gofpdf

A PDF document generator with high level support for text, drawing and images
http://godoc.org/github.com/jung-kurt/gofpdf
MIT License
4.31k stars 777 forks source link

fpdi.Importer wrapper for thread-safe usage #269

Closed ayvan closed 5 years ago

jung-kurt commented 5 years ago

Thanks, @ayvan. This is a good change.

@phpdave11, there are a couple of options here. At the least, we should add documentation to the existing functions to indicate that for concurrent use, the wrapped importer should be used. In this case, I think it makes sense to factor out the content in the existing ImportPage() and call the wrapped version inside. The global variable fpdi would still be present, but it would be assigned from an importer that is returned from NewFpdiImporter().

The more extreme option would be to make a breaking change and switch over entirely to the multiple goroutine-safe version. Your thoughts?

ayvan commented 5 years ago

I think, it's possible to make no-breaking changes that will support goroutine-safe work. I need some time to do this ...

phpdave11 commented 5 years ago

@jung-kurt I'm thinking that we shouldn't duplicate those functions if possible. If the code can be modified to allow for concurrent use, that would be my vote. Thanks @ayvan!

ayvan commented 5 years ago

I have made a number of improvements in this branch: https://github.com/ayvan/gofpdf/blob/dev/contrib/gofpdi/gofpdi.go

But I think this is still not the best solution. Potentially map of importers is really can leaks memory, and we must delete importers from map after usage.

ayvan commented 5 years ago

Any ideas about https://github.com/ayvan/gofpdf/commit/50632cb277a95d0383dd6158170bc3b2502a7bda#diff-f4a5b0b4df933cb5464904a786390c03 ?

I think, we need to clean map to avoid memory leaking, but have no ideas, how to do it, because we don't know, is this importer already used and is no longer needed.

jung-kurt commented 5 years ago

I am inclined to get rid of the global variable, mutex, and unsafe fields. Let the application call gofpdi.NewImporter() and store the return value locally. This way each goroutine can obtain its own importer.

ayvan commented 5 years ago

To maintain backward compatibility, we need to keep separate copies of importers, just as one importer is currently stored: var fpdi = realgofpdi.NewImporter ()

Well, then you need to break backward compartibility and remove old functions.

jung-kurt commented 5 years ago

Well, then you need to break backward compartibility and remove old functions.

This will be @phpdave11's call. Maybe it's just a matter of keeping the existing global importer but documenting its shortcoming and explaining how NewImporter() can be used to get a local instance.

mrtsbt commented 5 years ago

What's the status of this proposal? I'd llike this feature, too. @ayvan are you still actively working on this? If not, I'd try to tackle this myself, based on your approach.

phpdave11 commented 5 years ago

@mrtsbt that would be great if you wanted to finish up the work on this. It would be nice to have backward compatibility if possible. Thanks!

mrtsbt commented 5 years ago

I picked up on this and created a pull request that hopefully achieves both goals: https://github.com/jung-kurt/gofpdf/pull/288