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

Unsatisfactory error recovery story with Response::end() #427

Open
abonander opened this issue Jan 28, 2018 · 3 comments
Open

Unsatisfactory error recovery story with Response::end() #427

abonander opened this issue Jan 28, 2018 · 3 comments

Comments

@abonander
Copy link
Contributor

If Response::end() fails, the response is consumed so there's no way to create a NickelError without unsafe (except that NickelError has all fields public so it's possible to bypass the constructor completely). The reasoning for unsafe on NickelError::without_response() talks about deadlock if the response is not flushed, which is what the user is trying to do by calling Response::end(). So if the flush fails, what are we meant to do? It seems like a guaranteed deadlock since we can't call bail() now.

@abonander
Copy link
Contributor Author

After some thought, it's occurred to me that Response::end() isn't meant to be called in client code. This should be documented on the method or it should be hidden entirely or marked pub(crate).

@jolhoeft
Copy link
Member

I suspect you are right. It looks like it is just called by the Response::bail() method, and maybe should just be inlined there. Alternatively, it could just as easily take a &self. It is not necessary to consume the response since all it does is flush the underlying hyper Response.

@Ryman
Copy link
Member

Ryman commented Feb 17, 2018

I think you're both correct, it existed before pub(crate) was an option and I also think it might not be as necessary anymore now, it used to be that dropping the Response would mean the connection would hang but hyper was updated to do the right thing on drop. As for NickelError::without_responsebeing unsafe, forcing the user to always return *something* which contained the response stream was the idea behind it and it's only really still a problem if theymem::forget` the Response I think which may lead to the hung stream... not sure if timeouts handle that now or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants