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: Knex CLI calls process.chdir() before opening Knexfile #3661

Merged
merged 2 commits into from Feb 13, 2020

Conversation

@briandamaged
Copy link
Collaborator

briandamaged commented Feb 12, 2020

Addresses the issue outlined here:

#3660

Short story: the Knex CLI was opening the Knexfile before calling process.chdir(...). Unfortunately, the Knexfile in question was resolving some paths relative to process.cwd(). As a result, these paths ended up being relative to the wrong directory.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 12, 2020

Wasn't your preferred approach not to change process directory altogether?

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 12, 2020

@kibertoad : It is; however, it looks like that is going to take a bit of work. It looks like several modules within Knex are implicitly coupled with process.cwd(). So, it will probably take a little while to revise those areas.

In the meantime, the current PR at least fixes this specific regression. (But yeah -- arguably, the Knexfile itself should not be calculating paths relative to process.cwd() since this value will be inconsistent. For example: process.cwd() will be different if you load the Knexfile from your own application instead of the Knex CLI)

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 13, 2020

@kibertoad : Hmm... the Travis CI failure seems to be an unrelated transaction failure. Is there an easy way to kick off the job again?

... this was mostly to kick off another Travis CI build since
the previous one seems to have failed randomly.
@kibertoad kibertoad merged commit 2c206a8 into knex:master Feb 13, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 13, 2020

Thanks! I'll publish version with hotfix in the evening.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 13, 2020

@kibertoad : Thx! And yeah, I'll look into the "decouple the code from process.cwd()" thing in the next few days. After skimming through the code a bit, it looks like there's implicit coupling in at least half a dozen different places. So, it's gonna take a little effort to figure out how to implement the change wo/ breaking backward compatibility in the process.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 13, 2020

Sorry, there will be a delay in releasing new version, there is a possible regression in timeout code that need investigation :-/

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 13, 2020

np!

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 14, 2020

@kibertoad : In terms of the timeouts, are you referring to the latest Travis-CI failure?

https://travis-ci.org/knex/knex/jobs/650139383?utm_medium=notification&utm_source=github_status

If so, I noticed that error recently as well. It seems to be one of those fun "random" errors...

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 14, 2020

@kibertoad : I think I've tracked down at least part of the problem. Take a look at the following:

knex/lib/runner.js

Lines 259 to 265 in 77df705

.then(async (connection) => {
try {
return await cb(connection);
} finally {
await this.client.releaseConnection(this.connection);
}
});

Suppose that await cb(connection) raises an unexpected error. Further suppose that await this.client.releaseConnection(this.connection) also raises an error. In this case, this 2nd error will interrupt the 1st error before it has been properly handled, resulting in an "unhandled rejection".

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 14, 2020

@maximelkin thoughts on explanation above?

@maximelkin

This comment has been minimized.

Copy link
Collaborator

maximelkin commented Feb 14, 2020

image

async function f() {
 try {
    await Promise.reject(new Error('a1'));
  } finally {
  await Promise.reject(new Error('b2'));
 }
}
f().catch(console.error)
@maximelkin

This comment has been minimized.

Copy link
Collaborator

maximelkin commented Feb 14, 2020

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.