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

Fixes Connection Reuse for Ember Client #3598

Conversation

ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Jul 22, 2020

Fixes #3592

First Error Discovered: When we are actually able to reuse a connection it was draining the root stream of extra bytes. As we had listed to take 2060 bytes, and then with the same reference try to pull 2060 more bytes as the effect was executed in the use block. This hung waiting for new bytes. (This signals to me that the user has a hold of the root connection not only a handle of 2060 bytes worth of it as pulling twice tries to take that twice, unsure if we should do something or how to do something that separates that logic). This means as well that a partially consumed stream will trigger the finalizers for reuse but the underlying stream will have however many bytes are uncomsumed present. We could add something that measures how much is read and then read that on shutdown, but then that introduces a race condition between release and an asynchronous effect. I feel that this default is reasonable for now, but we may want to revisit it in the future.

When introducing the host logic, we created a way to write the Host header twice, as if it was present in the headers and the uri it was written twice. This is now fixed to only render a Host header from a Uri if it is not present in headers. This is how its done in blaze but feels pretty inefficient compared to my other methods here. (We were previously ok due to how headers ++ operated)

@ChristopherDavenport
Copy link
Member Author

ChristopherDavenport commented Jul 22, 2020

@BalmungSan if you can confirm this fixes the issue for you that would be great. Mine now runs infinitely.

@BalmungSan
Copy link
Contributor

BalmungSan commented Jul 22, 2020

@ChristopherDavenport I can confirm that using a local publish of this branch everything works fine!
Once again, thanks for all the hard work!

(@d2a4u you may want to know about this)

@d2a4u
Copy link

d2a4u commented Jul 23, 2020

Amazing, thanks. I found this issue as well last night but was struggling to fix it by myself. Should have jumped on gitter sooner. Thanks.

hamnis
hamnis approved these changes Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants