Skip to content

Fix strange code, add messages to code brought in #8106#13509

Merged
jesusvazquez merged 1 commit intoprometheus:mainfrom
proggga:mfesenko/fix
Feb 2, 2024
Merged

Fix strange code, add messages to code brought in #8106#13509
jesusvazquez merged 1 commit intoprometheus:mainfrom
proggga:mfesenko/fix

Conversation

@proggga
Copy link
Copy Markdown
Contributor

@proggga proggga commented Feb 1, 2024

3-4 years ago I did a change for hackaton and today it was merged #8106 . I realized that I extracted method but in one place I extracted only half of function. Not sure this is good for code so I did a second PR (this) and added error messages because in any issues it will be hard to debug

@proggga proggga requested a review from jesusvazquez as a code owner February 1, 2024 14:51
@proggga proggga force-pushed the mfesenko/fix branch 2 times, most recently from 077dcaf to 284f086 Compare February 1, 2024 15:01
@proggga
Fix strange code, add messages to code brought in prometheus#8106
8a748ca 
Signed-off-by: Mikhail Fesenko <proggga@gmail.com>
@proggga proggga mentioned this pull request Feb 1, 2024
Merged
This was referenced Feb 2, 2024
Merged
Closed
pracucci
pracucci approved these changes Feb 2, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@pracucci pracucci left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for fixing it. I'm very surprised no test failed for this bug, which would have caused a corruption. I wouldn't block this PR until we get a test (cause merging this fix look high priority to me), but we should really get one.

@proggga
Copy link
Copy Markdown
Contributor Author

proggga commented Feb 2, 2024

Prob it works because it resets buffer but not sure, I'll try to add some tests here today evening

@pracucci
Copy link
Copy Markdown
Contributor

pracucci commented Feb 2, 2024

Prob it works because it resets buffer but not sure, I'll try to add some tests here today evening

Uhm, not sure. The writeAt() is called one extra time with the extra length 🤔

@proggga
Copy link
Copy Markdown
Contributor Author

proggga commented Feb 2, 2024

Agree, so I'm kinda not sure how it passed this test

jesusvazquez
jesusvazquez approved these changes Feb 2, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the fix! As marco said too bad a test did not catch this. If you are willing to add another PR for the test that would be awesome

@jesusvazquez jesusvazquez merged commit 419dd26 into prometheus:main Feb 2, 2024
@pracucci
Copy link
Copy Markdown
Contributor

pracucci commented Feb 2, 2024 •
edited
Loading

I think I may have understood why no test was failing. The length was written twice, but both times at the same position, overwriting the alen (4 bytes) placeholder initially written by writeLabelIndexesOffsetTable(). The writeLabelIndexesOffsetTable() function initially writes 4 bytes placeholder, then the length is later written back in the position of the placeholder. Writing the length twice was overwriting the placeholder twice, which is superfluous but actually not a corruption bug.

@proggga proggga deleted the mfesenko/fix branch February 2, 2024 09:28
@proggga
Copy link
Copy Markdown
Contributor Author

proggga commented Feb 2, 2024

Ah looks like you're right, it does w.write ... startPos, the same pos to write, so it re-write over and doesn't break anything, just writing 2 times

@pracucci
Copy link
Copy Markdown
Contributor

pracucci commented Feb 2, 2024 •
edited
Loading

Even the written length should have been correct, because w.f.pos (used to compute the length as l := w.f.pos - startPos - 4) was not advanced by .writeAt() (the .Write() advances it, but not the WriteAt()).

@proggga
Copy link
Copy Markdown
Contributor Author

proggga commented Feb 2, 2024

Can you please point me in doc, becuase I can't find any piece of doc saying about updateing position. Just need that to learn

@pracucci
Copy link
Copy Markdown
Contributor

pracucci commented Feb 2, 2024 •
edited
Loading

Can you please point me in doc, becuase I can't find any piece of doc saying about updateing position. Just need that to learn

My reasoning:

  • The length of an index section is computed by l := w.f.pos - startPos - 4 (code)
  • Before merging this PR we were computing the length twice. If w.f.pos changes between the 2 lines of code where we compute the length, then the 2nd length value would be corrupted.
  • The length is written by calling w.writeAt() (code), so question is: does w.writeAt() increases w.f.pos or not? I don't think so, because pos is increased by FileWriter.Write() (code) but not by FileWriter.WriteAt() (code)

@proggga
Copy link
Copy Markdown
Contributor Author

proggga commented Feb 2, 2024

i see, interesting

@proggga
Copy link
Copy Markdown
Contributor Author

proggga commented Feb 2, 2024

Postmortem:
Where did we get lucky - we didn't broke anything yet, even bug was introduced =)

@jesusvazquez
Copy link
Copy Markdown
Member

jesusvazquez commented Feb 2, 2024

Thanks both for caring about this so much 👍

fbegyn pushed a commit to fbegyn/prometheus that referenced this pull request Jul 11, 2024
@proggga @fbegyn
Fix strange code, add messages to code brought in prometheus#8106 (pr…
3d14929 
…ometheus#13509)

Signed-off-by: Mikhail Fesenko <proggga@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jesusvazquez jesusvazquez jesusvazquez approved these changes

+1 more reviewer

@pracucci pracucci pracucci approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@proggga @pracucci @jesusvazquez