-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fetching content from HTTPS using rustls
#2024
Conversation
Conflicts: Cargo.lock dapps/src/handlers/fetch.rs
@@ -67,7 +67,7 @@ impl<T> Hashable for T where T: BytesConvertable { | |||
} | |||
|
|||
/// Calculate SHA3 of given stream. | |||
pub fn sha3<R: io::Read>(r: &mut R) -> Result<H256, io::Error> { | |||
pub fn sha3<R: io::Read>(r: &mut io::BufReader<R>) -> Result<H256, io::Error> { |
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.
Isn't there a BufRead
trait for this?
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.
👍
needs rebase |
Conflicts: Cargo.lock
Conflicts: Cargo.lock
} | ||
|
||
fn is_aborted(&self) -> bool { | ||
self.abort.load(Ordering::Relaxed) |
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.
Really should use SeqCst
everywhere unless there's a good reason not to. It's not slower on x86
but it is not worth risking memory ordering bugs (which are awful) just to shave off a couple nanoseconds on other architectures.
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.
There might be only one state transition here: false->true
and even if it's not correctly detected there are no particularly bad consequences (the content will be just fetched till the end and removed after that).
I'm ok with SeqCst
- it's not hot code at all and if we can avoid some bugs because of that then 👍 :)
Related: #1176