Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the dns resolution of multiple hosts #2393

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

YuppY
Copy link

@YuppY YuppY commented Oct 27, 2020

This fix allows to use native client with multi-host configuration:

const { native: { Client } } = require('pg');

const client = new Client({
  user: 'user',
  host: 'foo,bar,baz',
  database: 'db',
});
client.connect().then(() => {
  client.query('SELECT 1', console.log);
});

This is workaround for #1470

I have a couple of questions though:

  1. Do we need this dns resolution at all? In which cases libpq cannot resolve host by itself?
  2. To make better tests of dns errors I need to patch dns.resolve. Can I just add jest to dependecies? Or node-postgres unit tests have to be written without mocking?

@brianc
Copy link
Owner

brianc commented Nov 2, 2020

Hey thanks for the pull request!

1. Do we need this dns resolution at all? In which cases libpq cannot resolve host by itself?

AFAIK libpq doesn't offer a non-blocking way to do dns resolution, so letting libpq do the dns resolution results in the node event loop blocking. There are ways to run blocking C/C++ operations on background threads with node bindings to get around this, but at the time that was over my head so I just did the resolving in node.

2\. Or `node-postgres` unit tests have to be written without mocking?

Yeah just monkey-patch dns.resolve in the test to a mock or stub function and then revert it to the original implementation at the end of the test if you need it to go back. Each file runs in its own node process so you don't have to do a ton of cleanup. Adding jest is a no-go...it would require way too much reworking of everything right now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants