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

SPDY parser.js tests #3

Closed
wants to merge 34 commits into from

Conversation

daviddias
Copy link
Member

Hi @indutny putting this here as a WIP. I (finally) got the test case working, however noticed that for a SYN_FRAME (type 0x01), the callback was being called with type being "HEADERS".

Also, found this - https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/protocol/spdy/parser.js#L184 which I guess to be leftovers from http2, at least I can't find any reference on the SPDY spec about a PUSH_PROMISE type of frame. Same goes for PRIORITY https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/protocol/spdy/parser.js#L191

Will continue working on the other frame tests, just wanted to keep you posted :)

@@ -172,7 +173,7 @@ Parser.prototype.onSynHeadFrame = function onSynHeadFrame(type,

if (!isPush) {
callback(null, {
type: 'HEADERS',
type: 'SYN_STREAM',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to break everything! ;) I did it for compatibility with HTTP2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, am I interfering with your idea/plan? Feel free to enlighten me :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with just HEADERS for both SYN_STREAM and SYN_REPLY. HTTP2 is using just HEADERS for both of them, and for continuation headers too. There is no complexity in handling this in connection.js.

TL;DR - let's leave it as it is ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, does that mean that it sends back a HEADERS type code instead of the SYN REPLY? Didn't notice that, or does this just happens on the parsing part.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The framer has them apart: requestFrame, responseFrame, headersFrame. The particular implementation chooses the right frame type.

@indutny
Copy link
Collaborator

indutny commented Jun 28, 2015

Yay!

@daviddias
Copy link
Member Author

What about the PUSH_PROMISE and PRIORITY?

@indutny
Copy link
Collaborator

indutny commented Jun 28, 2015

@diasdavid what about it?

@daviddias
Copy link
Member Author

Is it a compatibility thing too? (The fact they are around on the spdy/parser.js)

@indutny
Copy link
Collaborator

indutny commented Jun 28, 2015

Yeah, exactly. The protocols should look completely the same to the connection and stream.

@daviddias daviddias mentioned this pull request Jun 28, 2015
@whyrusleeping whyrusleeping mentioned this pull request Jun 29, 2015
34 tasks
@whyrusleeping whyrusleeping mentioned this pull request Jul 1, 2015
44 tasks
@indutny
Copy link
Collaborator

indutny commented Jul 1, 2015

Hello @diasdavid ! Do you need any help with this?

}, done);
})

it('should parse frame with http header and Unidirectional flag', function(done) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When 0x02 = FLAG_UNIDIRECTIONAL is passed, nothing is declared on the frame parsed. Is this intended @indutny ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fixed it, thanks for reminding!

@daviddias
Copy link
Member Author

Any feedback on things that could be done to make this better? :) All tests pass now, code style is followed. Frames were extracted from go<->node experiments (which all of them worked :) )

@@ -190,7 +190,7 @@ Parser.prototype.onSynHeadFrame = function onSynHeadFrame(type,

if (!isPush) {
callback(null, {
type: 'HEADERS',
type: 'HEADERS', // type === 0x01 ? 'SYN_STREAM': 'SYN_REPLY',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous change.

@indutny
Copy link
Collaborator

indutny commented Jul 7, 2015

Looks pretty good except some style nits! Thank you!

@daviddias
Copy link
Member Author

Awesome :) fixed all of the things you mentioned :)

@indutny
Copy link
Collaborator

indutny commented Jul 7, 2015

Landed in 4eb339b, thank you so much!

@indutny indutny closed this Jul 7, 2015
@daviddias
Copy link
Member Author

wooo :D Awesome, thank you!

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

Successfully merging this pull request may close these issues.

2 participants