Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

feat: throw exception if reads corrupt data #863

Merged
merged 11 commits into from
Jul 25, 2021

Conversation

levy5307
Copy link
Contributor

@levy5307 levy5307 commented Jul 22, 2021

In previous implementation, server will coredump if it reads corrupt data.
In this pull request, I changed the way to throw exception. So someone can catch the exception if he don't want to make server crash.

@levy5307 levy5307 changed the title fix:throw exception if reads corrupt data fix: throw exception if reads corrupt data Jul 22, 2021
@levy5307 levy5307 changed the title fix: throw exception if reads corrupt data refactor: throw exception if reads corrupt data Jul 22, 2021
@hycdong
Copy link
Contributor

hycdong commented Jul 23, 2021

I think feat may be better than refactor to describe this pull request.

@levy5307 levy5307 changed the title refactor: throw exception if reads corrupt data feat: throw exception if reads corrupt data Jul 23, 2021
@@ -39,21 +40,27 @@ class binary_reader
int read(/*out*/ bool &val) { return read_pod(val); }

int read(/*out*/ std::string &s);
int read(char *buffer, int sz);
virtual int read(char *buffer, int sz);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's an important path on write, add a virtual function will add a performance cost in c++, is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The performance cost introduced by vtable can be negligible. And this pull request is used for dealing with dirty mutation log.

@levy5307 levy5307 merged commit 7e3eab7 into XiaoMi:master Jul 25, 2021
@levy5307 levy5307 deleted the rpc-coredump branch July 25, 2021 10:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants