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

config: make I/O operations synchronous #1110

Merged
merged 2 commits into from
Jul 18, 2024
Merged

config: make I/O operations synchronous #1110

merged 2 commits into from
Jul 18, 2024

Conversation

jordigh
Copy link
Contributor

@jordigh jordigh commented Jul 17, 2024

Original commit messages:

  • a27eb98 config: replace fse functions with sync variants

    I need to be able to read the config at module load time, which makes
    async difficult if not impossible.

    This will make all config operations synchronous, which is fine. The
    file is tiny and seldom read or written to.

  • 3268077 config: remove IReadableConfigValue type

    This type is only used to define the IWritableConfigValue type, and
    since we are now doing synchronous read and writes, there's no point
    in separating get from set.

  • bfa045b config: remove all async/await around config functions

    Now that I/O is synchronous, there's no need to have any more
    async/await in regards to the config functions.

I need to be able to read the config at module load time, which makes
async difficult if not impossible.

This will make read config operations synchronous, which is fine. The
file is tiny and seldom read.
@jordigh jordigh force-pushed the jordigh/sync-config branch from 2209ad5 to ed1416c Compare July 17, 2024 19:55
@jordigh jordigh marked this pull request as ready for review July 17, 2024 20:46
Now that reading is synchronous, there's no need to have any more
async/await in regards to the those config functions.
@jordigh jordigh force-pushed the jordigh/sync-config branch from ed1416c to bfa045b Compare July 17, 2024 21:48
Copy link
Contributor

@Spoffy Spoffy left a comment

Choose a reason for hiding this comment

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

Generally looks fine on the read side, though I think it would be valuable to keep the write side asynchronous.

Read is done primarily during startup, so the impact of a slow file read is minimal.

Write however can be done at any point during operation, and a slow write could hang the entire server. So I think it's worth keeping write async

@jordigh jordigh force-pushed the jordigh/sync-config branch 2 times, most recently from 8e57b34 to 5f23415 Compare July 18, 2024 17:57
@jordigh
Copy link
Contributor Author

jordigh commented Jul 18, 2024

Updated commit messages:

  • 971b9c0 config: replace fse read functions with sync variants

    I need to be able to read the config at module load time, which makes
    async difficult if not impossible.

    This will make read config operations synchronous, which is fine. The
    file is tiny and seldom read.

  • 5f23415 config: remove all async/await around config read functions

    Now that reading is synchronous, there's no need to have any more
    async/await in regards to the those config functions.

@jordigh jordigh merged commit e30a090 into main Jul 18, 2024
11 checks passed
@jordigh jordigh deleted the jordigh/sync-config branch July 18, 2024 18:32
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