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

sheet extension and index cleanup #350

Merged
merged 7 commits into from
Dec 19, 2024
Merged

Conversation

liquidaty
Copy link
Owner

fix possible race condition w uibuff->index_started on extension build, rebuild libzsvutil if stale
to do:
consolidate index.c and transformation.c
remove index.h from external api
review writer.c for performance

fix possible race condition w uibuff->index_started
on extension build, rebuild libzsvutil if stale
to do: consolidate index.c and transformation.c
       remove index.h from external api
       review writer.c for performance
remove index.c from libzsvutil
@@ -207,6 +207,8 @@ static int read_data(struct zsvsheet_ui_buffer **uibufferp, // a new zsvsheet_
pthread_mutex_lock(&uibuff->mutex);
char need_index = !uibuff->index_started && !uibuff->write_in_progress;
char *old_ui_status = uibuff->status;
if (need_index)
uibuff->index_started = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering; I don't think there should be a race condition here because the UI buffer should not have a worker attached at this point and this is executing in the main thread?

Copy link
Owner Author

Choose a reason for hiding this comment

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

You're probably right but this way we don't have to think about it, or worry about some change in the future adding a worker before this block executes

@liquidaty liquidaty merged commit 79e769b into main Dec 19, 2024
14 checks passed
@liquidaty liquidaty deleted the extension-and-index-cleanup branch December 19, 2024 22: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