Skip to content
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

Add Mutex in HttpError to make Error sync again #406

Closed
wants to merge 1 commit into from

Conversation

djmcgill
Copy link

@djmcgill djmcgill commented Oct 3, 2018

Closes #362 and provides a significant usability boost when using failure.

This is an API breaking change so will need to wait for a major version bump. If you'd rather I place it on the 0.6.x branch I can do that instead.

@arqunis arqunis added enhancement An improvement to Serenity. fix A solution to an existing bug. labels Oct 3, 2018
@arqunis arqunis assigned ghost Oct 3, 2018
@ghost ghost removed their assignment Nov 13, 2018
@troiganto
Copy link

I just found this PR because I stumbled over #362 myself. I looked a bit into this and ultimately, the error is not Sync because the hyper::client::Response contained in HttpError isn't. This is because reading the payload of the response is not thread-safe.

In my case, if such an error occurs, I'd like to print the reponse with status code and the response body. The solution in this PR, wrapping the entire struct in a mutex, would make this difficult because I'd be able to read the response only once, and it's gone afterwards.

What I'm wondering is: Would it make more sense to read the entire response into one or more Strings and include that in the error type? The type would then roughly look like this:

#[derive(Debug)]
pub enum Error {
    // Maybe include Headers and HttpVersion, too.
    UnsuccessfulRequest{ status: StatusCode, url: Url, body: String, },
    RateLimitI64,
    RateLimitUtf8,
}

I could hack together a PR for this, but I wanted to check first if there's any interest for that.

@arqunis arqunis added the http Related to the `http` module. label Mar 14, 2019
@Lakelezz Lakelezz mentioned this pull request Apr 11, 2019
18 tasks
@Lakelezz
Copy link
Contributor

Lakelezz commented May 1, 2019

Thanks for your pull request, but #532 ended up being a better solution to this problem!

@Lakelezz Lakelezz closed this May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement to Serenity. fix A solution to an existing bug. http Related to the `http` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error is not Send + Sync
4 participants