simonhaenisch / md-to-pdf

Hackable CLI tool for converting Markdown files to PDF using Node.js and headless Chrome.
https://www.npmjs.com/md-to-pdf
MIT License
1.16k stars 110 forks source link

Improve performance for concurrent conversions by re-using browser context #141

Closed simonhaenisch closed 1 year ago

simonhaenisch commented 1 year ago

Replaces #139.

Closes #138.

chamabreu commented 1 year ago

Nice, looks cleaner.

Didn't like my approach with the passing either 👍

chamabreu commented 1 year ago

Heyo,

just tested this Branch. Does not work. The Block

    if (!browser) {
        console.log('Have no browser, creating one');
        browser = await puppeteer.launch({ devtools: config.devtools, ...config.launch_options });
    }

Gets called many times in concurrent mode. You have to instantiate the Browser early on, because the launch takes to long, and in the time the function gets called multiple times without a browser.

I see no other posibility than creating a browser instance at a higher level, await this, and then start the conversion processes.

As I mentioned, maybe a class based approach could help here. What do you think?

chamabreu commented 1 year ago

Heyo,

just tested this Branch. Does not work. The Block

  if (!browser) {
      console.log('Have no browser, creating one');
      browser = await puppeteer.launch({ devtools: config.devtools, ...config.launch_options });
  }

Gets called many times in concurrent mode. You have to instantiate the Browser early on, because the launch takes to long, and in the time the function gets called multiple times without a browser.

I see no other posibility than creating a browser instance at a higher level, await this, and then start the conversion processes.

As I mentioned, maybe a class based approach could help here. What do you think?

Possible Solution:

142

I tested also class based, but this should work also.

simonhaenisch commented 1 year ago

Ah yup makes sense and that actually explains the problem I was seeing with it hanging when running concurrent tasks! Because it created a bunch of browser instances simultaneously but only closed the last one. It was too late yesterday 😅 (I started falling asleep while working on this)

I just fixed it... didn't merge your PR though because it was missing the part where we don't need to call process.exit() anymore, but set you as the commit author so you'll get the credit 😊

simonhaenisch commented 1 year ago

All tests passing but @chamabreu if you could test this branch one more time with your 90 files I'd be even more confident to merge this 🤓