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

reset consumer #1535

Merged
merged 1 commit into from
Dec 2, 2021
Merged

Conversation

firesWu
Copy link
Contributor

@firesWu firesWu commented Nov 4, 2021

Reset consumer every time when finish one layer.

If consumer not reset, may cause parent cache loading problem.

  1. cur_parent_ptr may point to a released memory location, causing panic.
  2. cur_parent_ptr may point to a wrong parent cache window, resulting wrong pc1 result.

@qy3u
Copy link
Contributor

qy3u commented Nov 4, 2021

The bug you mentioned doesn't seem to be due to the consumer value.
The value of consumer has been correctly set to 1 here

@firesWu
Copy link
Contributor Author

firesWu commented Nov 4, 2021

The bug you mentioned doesn't seem to be due to the consumer value. The value of consumer has been correctly set to 1 here

The thread that calls create_label_runner was created before consumer being reset to 1. So we cannot be sure that the consumer used here and here is already reset to 1. @qy3u

@qy3u
Copy link
Contributor

qy3u commented Nov 4, 2021

I think you are right.
The consumer needs to be reset before create_label_runner.

@vmx
Copy link
Contributor

vmx commented Nov 4, 2021

I've only had a quick look, but to me it sounds like it is the right thing to do, to also reset the consumer.

vmx
vmx previously approved these changes Dec 2, 2021
Copy link
Contributor

@vmx vmx left a comment

Choose a reason for hiding this comment

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

@firesWu Sorry for the long delay. I know had a closer look. You change looks absolutely right. Thanks for spotting this issue. I was recently also deep into that part of the code, but I haven't spotted this issue. Good job!

Could you please rebase your change to current master (so that we don't end up with strange merge commits)?

@firesWu
Copy link
Contributor Author

firesWu commented Dec 2, 2021

@vmx I have rebase my change to current master

@cryptonemo cryptonemo merged commit bae20c1 into filecoin-project:master Dec 2, 2021
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.

5 participants