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

Creating client streams #1

Closed
daviddias opened this issue Jun 25, 2015 · 12 comments
Closed

Creating client streams #1

daviddias opened this issue Jun 25, 2015 · 12 comments

Comments

@daviddias
Copy link
Member

@indutny can you enlighten me on how to proper create a client stream?

Have been tried to get spdystream(go version) and spdy-transport to talk, but I believe I'm missing something on the options: https://github.com/diasdavid/spdy-cross-test/blob/master/node/client.js#L11-L13

As far for the go-client and node-server example, it throws a error in parser.js#L133, going to start looking into the spdy tests as you suggested here: spdy-http2/node-spdy#208 (comment)

UPDATE:

Just realized that it spdy-transport only supports 0.12 and above, so nvm the parser.js#L133 error.

Now I get:

events.js:85
      throw er; // Unhandled 'error' event
            ^
Error: Missing `:method` and/or `:path` header

I should be able to open a stream with an empty http header. Thoughts? Removing https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/protocol/spdy/parser.js#L152-L155 works, but I'm getting undefined undefined {} as the frame parsed.

@indutny
Copy link
Collaborator

indutny commented Jun 26, 2015

Have you tried: connection.request({ method: 'GET', host: 'localhost', path: '/' }, function(stream) { ... })?

@daviddias
Copy link
Member Author

@indutny we should be able to open a stream without a method and path header. HTTP semantics are part of the HTTP layering and not of the SPDY Framing layer itself. You probably know the spec by heart now, but just for reference, here you go: https://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3-1#TOC-3.2-HTTP-Request-Response

Nevertheless, I tried adding a proper header from the go client. The header got parsed correctly, but it failed since it expected :method and :path as keys.

{ method: 'GET', host: 'localhost', path: '/' } # I added a console.log on parser.js just to check if it got parsed well
events.js:85
      throw er; // Unhandled 'error' event
            ^
Error: Missing `:method` and/or `:path` header

If I remove the : from the if state, I get this on the server side:

undefined undefined { path: '/', method: 'GET', host: 'localhost' }

But the expected was to get "Writing to stream" instead of undefined. From the client:

    fmt.Fprint(stream, "Writing to stream")

@indutny
Copy link
Collaborator

indutny commented Jun 27, 2015

You meant this?

The first line of the request is unfolded into name/value pairs like other HTTP headers and MUST be present:
":method" - the HTTP method for this request (e.g. "GET", "POST", "HEAD", etc)
":path" - the url-path for this url with "/" prefixed. (See RFC3986). For example, for "http://www.google.com/search?q=dogs" the path would be "/search?q=dogs".
":version" - the HTTP version of this request (e.g. "HTTP/1.1")

I'm quite sure that there is not much point in creating stream without path, and you may use GET for a method name, because it is cheap to do so. If it turns out to be a big deal still, I'll probably make it optional or make it default to GET.

The reason why I do it this way is that I'm polyfilling between HTTP2 and SPDY, to make sure that this transport library behaves in completely same way when using any of these protocols.

@daviddias
Copy link
Member Author

You meant this?

The first line of the request is unfolded into name/value pairs like other HTTP headers and MUST be present:
":method" - the HTTP method for this request (e.g. "GET", "POST", "HEAD", etc)
":path" - the url-path for this url with "/" prefixed. (See RFC3986). For example, for "http://www.google.com/search?q=dogs" the path would be "/search?q=dogs".
":version" - the HTTP version of this request (e.g. "HTTP/1.1")

That is part of the HTTP layering over SPDY, so it shouldn't be mandatory for the framing layer, right?

It is true that we can always make path be "/", and method "GET" and open the stream anyways. Although it may look odd, it should definitely work good enough.

@indutny
Copy link
Collaborator

indutny commented Jun 27, 2015

I don't think that HTTP layering could be viewed separately from the SPDY spec.

SPDY is intended to be as compatible as possible with current web-based applications. This means that, from the perspective of the server business logic or application API, the features of HTTP are unchanged. To achieve this, all of the application request and response header semantics are preserved, although the syntax of conveying those semantics has changed. Thus, the rules from the HTTP/1.1 specification in RFC2616 apply with the changes in the sections below.

@indutny
Copy link
Collaborator

indutny commented Jun 28, 2015

Ok, what I just done is following. Framer now defaults to GET and localhost host if you do not supply them. This is insanely cheap for both SPDY and HTTP2 because of the compression.

@daviddias
Copy link
Member Author

rad :)

Would there a problem if the server assumed so too? So that it offers free interop with docker/spdystream (and other possible SPDY framing layer impl?)

Right now I'm running into a issue where I can connect a go spdystream client to a spdy-transport server, but a node spdy-transport client to a spdy-transport server breaks on the client.

# https://github.com/diasdavid/spdy-cross-test/blob/master/node/client.js
» node client.js                                                                                        
/Users/david/Documents/Code/ipfs/spdy-transport/lib/spdy-transport/protocol/spdy/framer.js:98
  this.compress.write(block.render(), callback);
               ^
TypeError: Cannot read property 'write' of null
    at Framer.headersToDict (/Users/david/Documents/Code/ipfs/spdy-transport/lib/spdy-transport/protocol/spdy/framer.js:98:16)
    at Framer._synFrame (/Users/david/Documents/Code/ipfs/spdy-transport/lib/spdy-transport/protocol/spdy/framer.js:176:8)
    at Framer.requestFrame (/Users/david/Documents/Code/ipfs/spdy-transport/lib/spdy-transport/protocol/spdy/framer.js:211:8)
    at Connection.request (/Users/david/Documents/Code/ipfs/spdy-transport/lib/spdy-transport/connection.js:459:26)
    at Socket.<anonymous> (/Users/david/Documents/Code/go-projects/src/spdy-cross-test/node/client.js:11:10)
    at Socket.g (events.js:199:16)
    at Socket.emit (events.js:104:17)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1001:10)

@indutny
Copy link
Collaborator

indutny commented Jun 28, 2015

I'm afraid that you will need to call .setVersion(3.1) in your client code. This is required because of the SPDY 3.1 vs SPDY 3 auto-detection.

Guess, I'm going to re-work it eventually, sorry for this!

@daviddias
Copy link
Member Author

Got it, thanks! However, still not getting lucky, now I get a RST_STREAM frame instead of a SYN_REPLY (I do get a SETTINGS frame first)

client
server

It seems pretty much the same as what happens in the tests, not figuring out why this is failing, thoughts?

Nevermind, I was using the version where I had changed the 'HEADERS' type to 'SYN_STREAM' and therefore breaking everything™

@whyrusleeping whyrusleeping mentioned this issue Jun 29, 2015
34 tasks
@daviddias
Copy link
Member Author

@indutny I believe I get why I can't fully establish a go spdystream to a node spdy-transport connection, spdy-transport server never replies to a SYN_STREAM with a SYN_REPLY and so in go spdystream, the stream never assumes a ready state, but in the node spdy-transport, it seems that implementation doesn't wait to receive that message to start writing to it.

@indutny
Copy link
Collaborator

indutny commented Jun 30, 2015

@diasdavid hm... do you call stream.respond() on server? It should totally send SYN_REPLY.

@daviddias
Copy link
Member Author

OH! Well, I tried with stream.respond() and without it, but now that you asked, I remembered of putting the stream.write() call inside the stream.on('readable') event and it worked. if I call stream.write right after stream.respond, the write never gets to the client. I went in such a twist of supposed debug, when it seems I was just using it wrong :(.

Shouldn't there be a better way to know when the stream is available to write than .on('readable'), because this means that if the client never writes something, I cannot tell that I can start writing.

Update: No need anymore to wait for .on('readable') due to: b5c02c3

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

No branches or pull requests

2 participants