Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fetching content from HTTPS using rustls #2024

Merged
merged 11 commits into from
Sep 2, 2016
Merged

Fetching content from HTTPS using rustls #2024

merged 11 commits into from
Sep 2, 2016

Conversation

tomusdrw
Copy link
Collaborator

Related: #1176

@tomusdrw tomusdrw added the A0-pleasereview 🤓 Pull request needs code review. label Aug 31, 2016
@@ -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> {
Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 31, 2016
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Aug 31, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.0%) to 84.356% when pulling 99acd49 on dapps-https into 6945dc3 on master.

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Aug 31, 2016
@arkpar
Copy link
Collaborator

arkpar commented Sep 1, 2016

needs rebase

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Sep 1, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.05%) to 84.356% when pulling 36c83a4 on dapps-https into b4f3c4b on master.

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 1, 2016
@rphmeier rphmeier added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 2, 2016
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Sep 2, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.02%) to 84.432% when pulling b5863cc on dapps-https into e0feaa9 on master.

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 2, 2016
}

fn is_aborted(&self) -> bool {
self.abort.load(Ordering::Relaxed)
Copy link
Contributor

@rphmeier rphmeier Sep 2, 2016

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.

Copy link
Collaborator Author

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 👍 :)

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.03%) to 84.428% when pulling f7da5fa on dapps-https into e0feaa9 on master.

@arkpar arkpar merged commit 59f18ab into master Sep 2, 2016
@tomusdrw tomusdrw deleted the dapps-https branch September 5, 2016 19:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants