Skip to content

Make the :sha256 set check resist race conditions #3180

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 5 commits into from
Jun 16, 2023

Conversation

invisig0th
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -3.95 ⚠️

Comparison is base (54d2d7e) 97.34% compared to head (10d0a54) 93.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3180      +/-   ##
==========================================
- Coverage   97.34%   93.40%   -3.95%     
==========================================
  Files         224      224              
  Lines       44836    44837       +1     
==========================================
- Hits        43644    41878    -1766     
- Misses       1192     2959    +1767     
Flag Coverage Δ
linux 93.40% <100.00%> (-3.85%) ⬇️
linux_replay ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
synapse/models/files.py 99.28% <100.00%> (+<0.01%) ⬆️

... and 46 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -183,8 +183,9 @@ async def initCoreModule(self):
async def _hookFileBytesSha256(self, node, prop, norm):
# this gets called post-norm and curv checks
if node.ndef[1].startswith('sha256:'):
Copy link

@ghost ghost Jun 13, 2023

Choose a reason for hiding this comment

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

It seems like this check is redundant (and maybe slower? Is .startswith() less performant than the f-string and comparison?). You could probably remove this line and outdent the three lines you added.

Just a suggestion, no change required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are instances where the string starts with guid: that we don't want to check for matching the norm'd fstring 👍

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I don't totally understand how this fixes the race but the change looks good to me.

@vEpiphyte vEpiphyte added this to the v2.13x.x milestone Jun 14, 2023
@vEpiphyte vEpiphyte added bug reqChangelog requires changelog labels Jun 14, 2023
@vEpiphyte vEpiphyte merged commit 44ccf2b into master Jun 16, 2023
@vEpiphyte vEpiphyte deleted the visi-filemerge-tweak branch June 16, 2023 20:47
@vEpiphyte vEpiphyte removed the reqChangelog requires changelog label Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants