Skip to content

Exit process as soon as CLI finishes #60

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

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

lbguilherme
Copy link
Contributor

Workaround for brianc/node-postgres#2907.

Currently on a Postgres database that doesn't have SSL enabled, kysely-codegen will first attempt to connect with SSL, fail, and then proceed to connect without SSL. After that it finished successfully, but it hangs before exiting before some TCP connection from the SSL attempt is still open.

After the CLI finishes successfully, let's just close the process. This saves a few annoying seconds for me.

@RobinBlomberg
Copy link
Owner

Interesting! Two questions:

  1. Could this cause any unwanted side effects, such as keeping a database connection open on exit?
  2. Is it not possible to gracefully exit the connections instead, eliminating the need for process.exit(0)?

If the answer to both is "no", I guess it may be merged, if it works as expected on my machine as well.

@lbguilherme
Copy link
Contributor Author

lbguilherme commented Feb 6, 2023

  1. No. process.exit will exit the current process and immediately free all of its resources, such as memory and open files/sockets. As this is a CLI and the job is already done at this point, there is no harm.
  2. pg.connect() throws and the object we could close is leaked. I sent a patch upstream fixing this, but it could take a while (Destroy connection if the server does not support SSL brianc/node-postgres#2908). Alternatively we could have a --ssl flag instead of blindly trying connect with or without ssl, but that would be a breaking change. Just to be clear, I'm talking about this function https://github.com/RobinBlomberg/kysely-codegen/blob/master/src/introspector.ts#L28.

This change is not pretty, but is harmless.

@RobinBlomberg
Copy link
Owner

Great points! Then this will likely be migrated.

Some SSL support will probably be implemented seperately as well, as requested in #36.

@RobinBlomberg RobinBlomberg merged commit f532612 into RobinBlomberg:master Mar 17, 2023
@RobinBlomberg
Copy link
Owner

Released in [email protected]!

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