coopernurse / node-pool

Generic resource pooling for node.js
2.37k stars 259 forks source link

Question about my pool setup #231

Closed lsbyerley closed 6 years ago

lsbyerley commented 6 years ago

I am using generic-pool to run multiple instances of puppeteer and I'm curious if my setup is correct. One thing I've noticed is that it never gets into the destroy method of the factory, although I'm not sure if it actually needs to? I've seen the puppeteer-pool repo but it doesn't seem to be maintained anymore

// browserPagePool.js

const genericPool = require("generic-pool");
const puppeteer = require("puppeteer");
const browserPromise = async () => await puppeteer.launch({ headless: true });

const factory = {
  create: async function() {
    const browser = await browserPromise();
    const page = await browser.newPage();
    return page;
  },
  destroy: function(puppeteer) {
    puppeteer.close();
  }
};

const browserPagePool = genericPool.createPool(factory, {
  max: 10,
  min: 2,
  maxWaitingClients: 50
});

module.exports = browserPagePool;

// api route using the pool

const browserPagePool = require("../utils/browserPagePool");

router.get("/api/scrape", async (req, res, next) => {
  try {

    const page = await browserPagePool.acquire();

    await page.goto(url, { waitUntil: "networkidle0", timeout: 10000 });

    const content = await page.content()

    await browserPagePool.release(page); // is release supposed to get into destroy()?

    return res.status(200).json(content);

  } catch (err) {
    console.error(err);
  }
});
sandfox commented 6 years ago

You're correct, factory.destroy isn't gettting called 😄. When calling pool.release, the resource is put back into the pool of available resources so that it can be given out later. destroy would only get called when you "close" the pool. (all resources get destroyed at that point).

lsbyerley commented 6 years ago

@sandfox I see, thank you for clarifying! So is there anything you would change about my current setup?

As a side note, I'm guessing I need to add some sort of way to close the pool when/if the node process exits.

sandfox commented 6 years ago

you current setup seems mostly fine (I don't know much about puppeteer-pool so I can't comment much on thar part). I think your destroy function should be

  destroy: function(puppeteer) {
    return puppeteer.close();
  }

as puppeteer.close returns a promise and generic-pool can use/track that internally. and your create function can be simplified a bit

  create: async function() {
    const browser = await browserPromise();
    return browser.newPage();
  },

as generic-pool can handle and unwrap the promise from browser.newPage itself.

lsbyerley commented 6 years ago

@sandfox Thanks for your time and input, I'll add those recommendations

sandfox commented 6 years ago

forgot to add, when it comes to shutting down the pool:

https://github.com/coopernurse/node-pool#draining

/**
 * Step 3 - Drain pool during shutdown (optional)
 */
// Only call this once in your application -- at the point you want
// to shutdown and stop using this pool.
myPool.drain().then(function() {
  myPool.clear();
});

//or with async await

await myPool.drain()
await myPool.clear()

https://github.com/coopernurse/node-pool/blob/72b31a434c7b05ad879b2ace9830a9aa9fbba002/lib/Pool.js#L557-L565

https://github.com/coopernurse/node-pool/blob/72b31a434c7b05ad879b2ace9830a9aa9fbba002/lib/Pool.js#L593-L604