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: avoid synchronous calls where possible #1320

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Conversation

akx
Copy link
Contributor

@akx akx commented Jul 18, 2024

These were a bunch of *Sync calls that were rather easy to convert to promisified ones, either via util.promisify or just node:fs/promises.

There's at least one more place I didn't touch, solely because it would have required refactoring all of server.js::start to be async.

There are other places left (out of scope of this PR) where e.g. callback async could be converted to promises, etc.

@acalcutt
Copy link
Collaborator

acalcutt commented Jul 23, 2024

I would love another review of this, if anyone has the time. In general I think this is a good direction, but I am not very familiar with promises. I assume this makes things run a bit more in parallel so it would increase response times?

I reached out to @mnutt to see if he can offer a review, but I'm not sure if he is around right now and/or if he needs his permissions to this repo put back like I had to get done.

@akx
Copy link
Contributor Author

akx commented Jul 24, 2024

I assume this makes things run a bit more in parallel so it would increase response times?

It should decrease response times (that is, make things faster) where there are multiple concurrent requests to code paths which previously used *Sync calls (since a synchronous call will tie up the entire interpreter).

Signed-off-by: Aarni Koskela <akx@iki.fi>
Copy link
Collaborator

@acalcutt acalcutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and it seems to work fine. seems like a good direction to get away from synchronous calls

@acalcutt acalcutt merged commit 1d4c1ff into maptiler:master Jul 31, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants