-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Remove blocking/synchronous code inside async blocks #4
base: master
Are you sure you want to change the base?
Conversation
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.
Here's all places where blocking code is called.
@@ -329,17 +329,22 @@ fn main() -> Result<(), Error> { | |||
eprintln!("got {} new messages from server", packets.len()); | |||
|
|||
let previous: Option<Vec<u8>> = | |||
// This could block and get the entire async runtime to hang. | |||
db.get_entry_by_seq(&author, latest_seq).unwrap(); |
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.
Blocking code here.
db.get_entry_by_seq(&author, latest_seq).unwrap(); | ||
|
||
// Later, we should add stuff to store into about broken feeds in the db. | ||
// We should store why they broke and even store the offending message. | ||
// Then we can do a avoid trying to replicate broken feeds over and over. | ||
|
||
// This could block and get the entire async runtime to hang. | ||
par_validate_message_hash_chain_of_feed(&packets, previous.as_ref()).unwrap(); |
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.
Also here.
par_validate_message_hash_chain_of_feed(&packets, previous.as_ref()).unwrap(); | ||
eprintln!("validated messages"); | ||
|
||
// This could block and get the entire async runtime to hang. | ||
par_verify_messages(&packets, None).unwrap(); |
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.
Here.
par_verify_messages(&packets, None).unwrap(); | ||
eprintln!("verified messages"); | ||
|
||
// This could block and get the entire async runtime to hang. | ||
db.append_batch(&author, &packets).unwrap(); |
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.
And here.
let db = self.db.lock().unwrap(); | ||
let db = self.db.lock().await; | ||
|
||
// This could block and get the entire async runtime to hang. | ||
db.get_entries_newer_than_sequence( |
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.
And here.
This is critical as it could hang the entire runtime and/or affect performance very negatively.
It is pointless to use async blocks if it's to use synchronous code inside them, as the major benefit is performance, otherwise you're better off with using only synchronous code.
I understand that the underlying database adapter is synchronous, therefore, that one should be made asynchronous first.
async-std claims to offer a mechanism to dynamically detect and move blocking code off the main asynchronous runtime threads, this helps, but it's still bad for performance.