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

ssl_closed fix #473

Closed
wants to merge 1 commit into from
Closed

ssl_closed fix #473

wants to merge 1 commit into from

Conversation

AlexanderKai
Copy link

Here is a few lines of code that can fix up issue #464 in my opinion.

@benoitc
Copy link
Owner

benoitc commented Feb 7, 2018

I willl test it later today but looks good so far. Thanks for the patch!

@stephenmoloney
Copy link

stephenmoloney commented Feb 24, 2018

I tried to backport these changes to 1.10.1 but I am still running into the same error.
I can't upgrade to 1.11.0 as I keep running into the error on #447. I can reproduce the error in #447 and I'll post that over on that issue once i get the chance.

The local tests don't seem to output the error but building on travis does.

1.10.1 -> https://travis-ci.org/stephenmoloney/fastimage/builds/345600324

1.10.1 with ssl_fix -> https://travis-ci.org/stephenmoloney/fastimage/builds/345602341

p.s. The fastimage codebase is in a state of flux.

@AlexanderKai
Copy link
Author

@stephenmoloney
That fix don't solves issue #447.

@critch
Copy link

critch commented Feb 27, 2018

While testing this for #479 I ran into what might be a worse situation. While not crashing out with the unexpected error message, it will not return the done message. So in effect, the download loop will fail to exit do to never receiving the done message.

@AlexanderKai
Copy link
Author

AlexanderKai commented Feb 27, 2018

@critch
Can you send me a test case to reproduce that?

@benoitc
Copy link
Owner

benoitc commented Feb 28, 2018

have a look in #482 as well, that should probably fix the error but let me know.

@benoitc
Copy link
Owner

benoitc commented Apr 3, 2018

i commited 91aadac which should fix the issue. Thanks for the patch anyway :)

@benoitc benoitc closed this Apr 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants