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

Don't process the same transaction twice #560

Merged
merged 10 commits into from
Jun 29, 2022

Conversation

MTRNord
Copy link
Contributor

@MTRNord MTRNord commented Mar 29, 2022

This implements the transaction code handling as described in https://spec.matrix.org/v1.2/application-service-api/#pushing-events

TODO

  • Write Test(s)
  • Check if this is actually smart or totally performance hell :)

Note

This does not fix the UB we have with the sync_token on the next step I think this was fixed meanwhile?

@MTRNord
Copy link
Contributor Author

MTRNord commented Mar 29, 2022

One thing to consider is that receive_transaction is moved over to the appservice crate. But not sure if that will actually work with the store as the appservice stuff is in another crate and pub(crate) would make it not visible to the appservice

@MTRNord MTRNord changed the title Correctly handle the transaction token Don't process the same transaction twice Mar 29, 2022
@poljar
Copy link
Contributor

poljar commented Mar 29, 2022

One thing to consider is that receive_transaction is moved over to the appservice crate. But not sure if that will actually work with the store as the appservice stuff is in another crate and pub(crate) would make it not visible to the appservice

Is it? Where? In this PR?

AFAIR it can't live in the app-service crate unless we're willing to expose internal Client things, and this is a nice compromise which let's you receive responses without them being sent using the Client.

That being said, I'm not super familiar with app-services to say if that's the best approach.

@MTRNord
Copy link
Contributor Author

MTRNord commented Mar 29, 2022

One thing to consider is that receive_transaction is moved over to the appservice crate. But not sure if that will actually work with the store as the appservice stuff is in another crate and pub(crate) would make it not visible to the appservice

Is it? Where? In this PR?

AFAIR it can't live in the app-service crate unless we're willing to expose internal Client things, and this is a nice compromise which let's you receive responses without them being sent using the Client.

That being said, I'm not super familiar with app-services to say if that's the best approach.

Sorry I badly worded it. It was from the chat earlier and actually should say that it may be considered to do. Since then I came to the same conclusion though. Apart from the store thingy I added it cant be moved to the appservice crate.

@MTRNord MTRNord force-pushed the Famedly/appservice-transaction-token branch from c20d151 to 73c6250 Compare June 24, 2022 11:54
@MTRNord MTRNord force-pushed the Famedly/appservice-transaction-token branch from 38aafa8 to 90af3fa Compare June 24, 2022 11:59
@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #560 (e8318dc) into main (091fab8) will increase coverage by 0.26%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##             main     #560      +/-   ##
==========================================
+ Coverage   77.76%   78.03%   +0.26%     
==========================================
  Files          92       92              
  Lines       13675    13885     +210     
==========================================
+ Hits        10635    10835     +200     
- Misses       3040     3050      +10     
Impacted Files Coverage Δ
crates/matrix-sdk-appservice/tests/tests.rs 96.48% <96.66%> (+0.03%) ⬆️
crates/matrix-sdk/src/client/mod.rs 84.56% <100.00%> (-0.14%) ⬇️
crates/matrix-sdk-base/src/client.rs 78.45% <0.00%> (+1.13%) ⬆️
crates/matrix-sdk-base/src/rooms/normal.rs 81.35% <0.00%> (+1.59%) ⬆️
crates/matrix-sdk/src/room/common.rs 67.52% <0.00%> (+14.95%) ⬆️
crates/matrix-sdk-base/src/rooms/members.rs 50.00% <0.00%> (+17.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 091fab8...e8318dc. Read the comment docs.

@MTRNord MTRNord marked this pull request as ready for review June 24, 2022 12:24
@MTRNord
Copy link
Contributor Author

MTRNord commented Jun 24, 2022

Test fails seem unrelated?

Binding test was a stack overflow on setup and sso-login tests were a Connection reset error while running them

@jplatte
Copy link
Collaborator

jplatte commented Jun 24, 2022

Re-running failed jobs…

Copy link
Contributor

@johannescpk johannescpk left a comment

Choose a reason for hiding this comment

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

LGTM, just a small nit.

crates/matrix-sdk/src/client/mod.rs Show resolved Hide resolved
crates/matrix-sdk-appservice/tests/tests.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-appservice/tests/tests.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-appservice/tests/tests.rs Outdated Show resolved Hide resolved
@jplatte jplatte enabled auto-merge (squash) June 29, 2022 14:25
@jplatte jplatte merged commit a8601e1 into matrix-org:main Jun 29, 2022
@MTRNord MTRNord deleted the Famedly/appservice-transaction-token branch July 4, 2022 06:33
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.

4 participants