Skip to content

fix(pglt_workspace): lock #213

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
Feb 21, 2025
Merged

fix(pglt_workspace): lock #213

merged 1 commit into from
Feb 21, 2025

Conversation

psteinroe
Copy link
Collaborator

@psteinroe psteinroe commented Feb 21, 2025

fixes our bug from last night. the issue was actually that the entire server went into a lock when loading the SchemaCache for auto-completion, because we did not release the read lock on inner before acquiring the write lock. I wonder how we could write a test for these kind of issues 🤔 maybe some kind of simulation e2e test where we actually start the server and start to send some commands and then stop it again.

also removed some spammy logs.

Comment on lines +47 to +53
{
// return early if the connection string is the same
let inner = self.inner.read().unwrap();
if new_conn_str == inner.conn_str {
return Ok(SchemaCacheHandle::wrap(inner));
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we wrap this into its own block so the lock is dropped after

@psteinroe psteinroe changed the title fix: lock fix(pglt_workspace): lock Feb 21, 2025
Copy link
Collaborator

@juleswritescode juleswritescode left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning that up!

We could add a simple regression test to the schema cache manager?

@psteinroe
Copy link
Collaborator Author

Thanks for cleaning that up!

We could add a simple regression test to the schema cache manager?

Yeah in this case it should actually be sufficient to just call ist once 😂

@psteinroe
Copy link
Collaborator Author

Will do it in a follow up

@psteinroe psteinroe merged commit fb3f345 into main Feb 21, 2025
6 checks passed
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