-
Notifications
You must be signed in to change notification settings - Fork 38
Fix database importer decoding error #784
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
Conversation
|
|
||
| fn try_read_more(&mut self, bytes_to_read: usize) -> std::io::Result<usize> { | ||
| let mut addition = vec![0; bytes_to_read]; | ||
| fn read_more(&mut self, bytes_to_read: usize) -> std::io::Result<usize> { |
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.
No "not-try" methods, so it's probably not a try
| } | ||
|
|
||
| fn try_get_next_message_len(&mut self) -> Result<Option<usize>> { | ||
| fn decode_next_len(&mut self) -> Result<Option<(usize /*len*/, usize /*consumed*/)>> { |
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.
Return consumed to move the buffer/cursor/reader only in a single function when needed
| let mut cursor: &[u8] = &self.buffer; | ||
| match prost::decode_length_delimiter(&mut cursor) { | ||
| Ok(len) => { | ||
| let consumed = self.buffer.len() - cursor.len(); |
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.
cursor points to buffer
| Ok(bytes_read) if bytes_read >= required => {} | ||
| _ => return Some(Err(Error::Migration(MigrationError::CannotDecodeImportedConcept))), | ||
| let required = consumed + message_len; | ||
| while self.buffer.len() < required { |
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.
More read retries
|
|
||
| let mut message_buf = self.get_message_buf(message_len); | ||
| Some(M::decode(&mut message_buf).map_err(|_| Error::Migration(MigrationError::CannotDecodeImportedConcept))) | ||
| self.buffer.advance(consumed); |
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.
Advance in a single place
flyingsilverfin
left a comment
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.
To be honest I kinda skimmed it but if it works i'm happy!
Usage and product changes
Stabilize the database import function by eliminating rare decoding errors that could occur during the import of large datasets. All the exported files that the import function could not process are still valid and should be correctly imported after the proposed changes.
Implementation
VecDequewithprost::bytes::*and isolate the file reader's moving in a single function (Iterator::next).readfunctions can read fewer bytes than requested even without meeting EOF, replace singleifcalls withloops <- this was probably the main issue before.I was not able to reduce the bug to a single test case, even with thousands of records (however, we had a report of a failure after processing 200 entries), so no new behavior tests were introduced. I decided not to spend more time searching for such cases.