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

Implement request idempotency (fixes #4735) #5329

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Jan 16, 2025

If duplicate requests with the same Idempotency-Key are received, only the first one is processed and others receive an error.

https://www.ietf.org/archive/id/draft-ietf-httpapi-idempotency-key-header-01.html

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Nice, so then we just need to generate an idempotency UUID in the lemmy-js-client http code for puts and posts.

Should probably also do this for DELETE no?

I imagine CI is complaning about .expect, so you might need to either avoid those, or add a #[allow(clippy::expect_used)] to the functions.

let set: Arc<RwLock<HashSet<Entry>>> = Default::default();

let set_ = set.clone();
tokio::spawn(async move {
Copy link
Member

Choose a reason for hiding this comment

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

Ah good, as long as there's only one of these per server, not per hash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it must be shared across all server threads, otherwise it doesnt work.

@Nutomic
Copy link
Member Author

Nutomic commented Jan 17, 2025

The spec says that this only makes sense for CREATE and PUT. For example if you send the same "create post" multiple times, it will happily create multiple duplicate posts. But if you send the same "edit post", it will just set the same new text every time, which doesnt really matter. Same for delete, it would only throw an error that the item was already deleted which is fine.

// TODO: need to return LemmyError as well?
let response = HttpResponse::UnprocessableEntity()
.finish()
.map_into_right_body();
Copy link
Member Author

Choose a reason for hiding this comment

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

What about this, is status 422 enough or do we need a json error?

@dullbananas
Copy link
Collaborator

Seems like this won't always work in a horizontally scaled setup

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.

3 participants