-
Notifications
You must be signed in to change notification settings - Fork 24
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
Silently ignore duplicate tracing save requests #3767
Conversation
…scalableminds/webknossos into silently-ignore-duplicate-save-requests
…ently-ignore-duplicate-save-requests
@fm3 Thanks for pushing this, unfortunately this does not fix the issue for me yet, I'm still able to reproduce it. The way I'm testing it is as follows:
If you're having trouble reproducing this, we can also have a look at it together on Monday :) |
…ently-ignore-duplicate-save-requests
…rce singleton caches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍 The code looks good. I didn't test it now, since you two both did, right?
frontend/javascripts/libs/math.js
Outdated
// This module should be used to access the Math object, so it can be mocked in the unit tests | ||
// mockRequire("libs/math", myFakeMath); | ||
|
||
export default Math; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename this file to uid_generator.js
or something like that and then expose the request id generator, which can be mocked. This would limit the surface area of the mocked code a bit. Now, the mock for the save saga would need to be adapted as soon as we use more Math
methods in the save saga (similar to how Math.min
had to be re-added to the mock). Also, it suggests to always use this module instead of importing Math directly, but that's not really necessary in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's much better, thank you! :)
…ently-ignore-duplicate-save-requests
@youri-k could you have a look at the backend code? :) We already tested this, but it would be helpful if you could quickly double-check the code anyway. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM 🙂
@daniel-wer I wasn’t sure how to test this. You were able to reproduce the error in the first place, could you try if this fixes it? :) Also, my frontend changes might need a cleanup (I just dumped some randomstring generation) Thanks!URL of deployed dev instance (used for testing):
Steps to test:
Open tracing and dev console (chrome)
Set network speed to "Slow 3G"
Set a node and press "Save"
Then quickly check the "Offline" box, so that the request will reach the server, but the response won't reach the frontend ("Slow 3G" is helping with that)
Go online again and press "Save"
should continue without 409 error
normal tracing should be unchanged
tracing in multiple tabs should still give conflict errors and force reload
Issues:
[ ] Updated migration guide if applicable[ ] Updated documentation if applicable