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

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
Copy link
Collaborator

kibertoad commented Feb 12, 2020

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

@briandamaged
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
@kibertoad
Copy link
Collaborator

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

@briandamaged
Copy link
Collaborator Author

@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
Copy link
Collaborator

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

@briandamaged
Copy link
Collaborator Author

np!

@briandamaged
Copy link
Collaborator Author

@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
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
Copy link
Collaborator

@maximelkin thoughts on explanation above?

@maximelkin
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
Copy link
Collaborator

#3665

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.

None yet

3 participants