emitting :promise_headers when promise becomes available

[HoneyryderChuck]

Fixes #114

[coveralls]

Coverage increased (+0.02%) to 96.715% when pulling 6f6a777 on HoneyryderChuck:issue-114 into 4047315 on igrigorik:master.

[HoneyryderChuck]

@igrigorik there is something weird going on with the deep_dup implementation for the specs. In some cases, a deep dup'ed copy pased to a method which encodes headers will wipe out the payload from the test frames. I've been experiencing this very randomly with my promise header tests, but have verified that this also happens for the headers frame.

Try running:

> bundle exec rspec spec/client_spec.rb:78 spec/stream_spec.rb:28

This should fail depending on which spec is executed first. But if you trace the stream spec using byebug, you'll see that the payload is gone:

    26:         expect(@stream.state).to eq :open
    27:       end
    28:       it 'should transition to reserved (local) on sent PUSH_PROMISE' do
    29:         @stream.send PUSH_PROMISE.deep_dup
    30:         require "pry-byebug"; binding.pry
 => 31:         expect(@stream.state).to eq :reserved_local
    32:       end
    33:       it 'should transition to reserved (remote) on received PUSH_PROMISE' do
    34:         @stream.receive PUSH_PROMISE
    35:         expect(@stream.state).to eq :reserved_remote
    36:       end

[1] pry(#<RSpec::ExampleGroups::HTTP2Stream::StreamStates::Idle>)> PUSH_PROMISE
=> {:type=>:push_promise, :flags=>[:end_headers], :stream=>1, :promise_stream=>2, :payload=>""}

I thought at first that this was from my spec, but the same also happens for stream_spec.rb:22:

    18: 
    19:     context 'idle' do
    20:       it 'should transition to open on sent HEADERS' do
    21:         @stream.send HEADERS.deep_dup
    22:         require "pry-byebug"; binding.pry
 => 23:         expect(@stream.state).to eq :open
    24:       end
    25:       it 'should transition to open on received HEADERS' do
    26:         @stream.receive HEADERS
    27:         expect(@stream.state).to eq :open
    28:       end

[1] pry(#<RSpec::ExampleGroups::HTTP2Stream::StreamStates::Idle>)> HEADERS
=> {:type=>:headers, :flags=>[:end_headers], :stream=>1, :payload=>""}

Thoughts?

[igrigorik]

👍

[igrigorik]

The PR itself looks reasonable (thanks for wiring this up), but we obviously need to address the failing test.

TBH, not sure on the deep_dup bit.. I vaguely recall running into this way back when, but I can't recall what the underlying issue there was. There might be some funny business going on here with our deep_dup implementation as well, which we lifted from ActiveSupport.

[HoneyryderChuck]

I find them too frequent and hard to ignore. And as this already happens to the headers, it oughta be rethought a bit.

The first suggestion would be to move away from using deep-dup'ed hashes, and just go with helper methods which always return a fresh instance of the frame hash. You might have had your reasons to go with deep-dups, so I'd like to hear them before implementing anything.

Just so that we're clear, I don't have anything against it, I just think that we could get around by using language features instead of a ruby non-feature :)

[igrigorik]

The first suggestion would be to move away from using deep-dup'ed hashes, and just go with helper methods which always return a fresh instance of the frame hash. You might have had your reasons to go with deep-dups, so I'd like to hear them before implementing anything.

Well, to be fair, that's effectively what deep_dup is under the hood, right? I'm open to suggestions for how we can do this better! :)

[HoneyryderChuck]

I meant, helper methods which would return fresh instances, instead of relying on deep-dup to work as it should. So, instead of PUSH_PROMISE.deep_due everywhere, one would just have push_promise_frame. Rspec helps inn that by allowing such DSL injection.

What do you think?

[HoneyryderChuck]

@igrigorik I gave up rewriting the tests in the PR, as this will open a different can of worms. So let's keep this PR conservative and change the minimum?

I do think that the tests should be rewritten, preferable without deep-dup feature, and using helper methods returning fresh frame instances.

[igrigorik]

@HoneyryderChuck current PR seems to pull in some existing changes, could you rebase?

I do think that the tests should be rewritten, preferable without deep-dup feature, and using helper methods returning fresh frame instances.

That's fair. We can open a separate issue for that.

[HoneyryderChuck]

@igrigorik done.

And 👍 for the tests in a separate repo. On the other hand, is that a blocker for a release? (Maybe 0.9.x would make sense due to the changed :promise_headers key?)

[igrigorik]

Code LGTM.

For completeness, can we also update the readme and client example:

• https://github.com/igrigorik/http-2#server-push
• https://github.com/igrigorik/http-2/blob/master/example/client.rb#L57

WDYT?

[HoneyryderChuck]

@igrigorik done. Although, there's nothing specific regarding this callback in the README.

[igrigorik]

Cool, looks good. Thanks for the great work here!

And 👍 for the tests in a separate repo. On the other hand, is that a blocker for a release?

No, this is an existing issue. Let's file a separate thread for it.

(Maybe 0.9.x would make sense due to the changed :promise_headers key?)

sgtm.