Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add wsproxy implementation for key fetching#14917

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
sreya merged 15 commits intomainfromjon/wspkeys
Oct 14, 2024
Merged

Conversation

sreya
Copy link
Collaborator

@sreyasreya commentedOct 1, 2024
edited
Loading

This PR implements thewsproxy variant for a key cache. Along withjwt PR these are the final pieces before we can start wiring everything up.

@sreyasreya marked this pull request as ready for reviewOctober 1, 2024 23:30
This change streamlines the CryptoKey handling by utilizing thecodersdk package, thereby reducing code duplication and potentiallysimplifying maintenance efforts in the future.
- Implement `cryptokeys.Keycache` interface in `CryptoKeyCache`.- Introduce context management for graceful shutdowns.- Simplify function signatures and improve concurrency handling.- Ensure functions return errors when cache is closed.
- Use `cryptokeys.ErrKeyNotFound` for missing keys- Use `cryptokeys.ErrKeyInvalid` for invalid keys- Replace `xerrors` for streamlined error codes
@Emyrk
Copy link
Member

Spike's comments I think highlight the need to implement this once. With the changes#14928, can we reuse 1 implementation?

@sreya
Copy link
CollaboratorAuthor

@Emyrk I'm game to refactor this to use one implementation but honestly I'd like to get this initiative over the finish line. I'd consider the refactor tech debt which I can tackle concurrently to ensuring there are no larger issues with the overall feature while we dogfood it in dev. You chill with that?

Emyrk reacted with thumbs up emoji

@Emyrk
Copy link
Member

@Emyrk I'm game to refactor this to use one implementation but honestly I'd like to get this initiative over the finish line. I'd consider the refactor tech debt which I can tackle concurrently to ensuring there are no larger issues with the overall feature while we dogfood it in dev. You chill with that?

I can live with it 👍

sreya reacted with heart emoji

- Replaced sync/atomic with sync.Mutex and sync.Cond for better  concurrency control, avoiding frequent lock contention and reducing  potential for data races.- Removed separate fetch logic and integrated it into the main crypto  key retrieval flow to handle concurrent fetches properly.- Optimized the Close method to minimize wait time by broadcasting on  cond to notify waiting fetches when closed.
The unused `cryptokeys.Keycache` interface implementation check was removed from the `CryptoKeyCache` struct, as it was unnecessary and did not contribute to functionality. This streamlines the code and adheres to best practices for interface checks.
}

func (k *CryptoKeyCache) key(sequence int32) (codersdk.CryptoKey, bool) {
if sequence == latestSequence {
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're going to use a special int32 to represent latest, you might as well go whole hog and just insert the crypto key into the map under that special value, and dropk.latest

sreya reacted with thumbs up emoji
The `latest` field in `CryptoKeyCache` was redundant and has been removed to simplify the code. Instead, the latest crypto key is now directly accessed from the `keys` map using `latestSequence`. This change reduces unnecessary state management and potential for data inconsistency.
@github-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelOct 12, 2024
@matifalimatifali removed the staleThis issue is like stale bread. labelOct 12, 2024
@sreyasreya merged commit384873a intomainOct 14, 2024
26 checks passed
@sreyasreya deleted the jon/wspkeys branchOctober 14, 2024 19:04
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsOct 14, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@EmyrkEmyrkEmyrk left review comments

@spikecurtisspikecurtisspikecurtis approved these changes

Assignees

@sreyasreya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@sreya@Emyrk@spikecurtis@matifali

[8]ページ先頭

©2009-2025 Movatter.jp