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

use docker/spdystream again #9

Merged
merged 5 commits into from
Jun 15, 2015
Merged

use docker/spdystream again #9

merged 5 commits into from
Jun 15, 2015

Conversation

jbenet
Copy link
Owner

@jbenet jbenet commented Jun 1, 2015

spdystream has been updated since i last messed with it.
commit log promises important race fixes.
So this PR uses docker/spdystream again, without my fixes.
(it seems they handled what i fixed differently).

But this doesn't pass the peerstream tests :/ it deadlocks. not sure
if its on our side or spdystream's. (i suspect spdystream)

@jbenet
Copy link
Owner Author

jbenet commented Jun 1, 2015

Note: travis tests broken. run go test locally.

@whyrusleeping whyrusleeping mentioned this pull request Jun 2, 2015
58 tasks
@whyrusleeping
Copy link
Collaborator

and these tests pass with your version of spdystream?

@jbenet
Copy link
Owner Author

jbenet commented Jun 3, 2015

Yeah :/, but the version I have is broken too, as we discovered after testing it.

@whyrusleeping whyrusleeping mentioned this pull request Jun 9, 2015
50 tasks
@whyrusleeping
Copy link
Collaborator

we should try out https://github.com/SlyMarbo/spdy . maybe it doesnt have the bug we're seeing here?

@SlyMarbo, any comparisons between your lib and dockers spdystream?

@SlyMarbo
Copy link

I haven't done any comparison myself and I'm not familiar with Docker's package. I have no reason to believe you should have any issues with my package, but I'd recommend moving on to HTTP/2 if that's an alternative, since SPDY is being deprecated by a lot of people. https://github.com/bradfitz/http2 would be a great place to start, but if you're committed to SPDY give my package a go :)

@jbenet
Copy link
Owner Author

jbenet commented Jun 10, 2015

@SlyMarbo thanks for the response! we'll be moving to http2 soon, we're only using spdy because we can interop with node easier that way (no streams based http2 impl in node yet)

@whyrusleeping
Copy link
Collaborator

that may not have fixed it...

@whyrusleeping
Copy link
Collaborator

yeah... so that didnt fix it.. it just removed the deadlock. now i'm getting EOF's all over.

@jbenet
Copy link
Owner Author

jbenet commented Jun 15, 2015

@whyrusleeping fixed it! :) see: jbenet/spdystream@f8ff317

What doesn't make me feel great is that i can seemingly deadlock spdystream: jbenet/spdystream@sync-stream-creation-test though not sure if I'm doing something stupid

jbenet added a commit that referenced this pull request Jun 15, 2015
@jbenet jbenet merged commit 72e1e9c into master Jun 15, 2015
@jbenet jbenet deleted the docker/spdystream branch June 15, 2015 08:59
@whyrusleeping
Copy link
Collaborator

Thats... concerning. You gonna PR against docker?

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.

3 participants