Fix double resume FiberError when stubbing .to_timeout with em-synchrony #95

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@JonRowe
JonRowe commented May 20, 2011

Hi,

The current implementation of errors/timeout for the em-http adaptor causes a double resume FiberError when used with em-synchrony.

See this gist to replicate the problem.

Adding EM.next_tick to the error/timeout throwing fixes the problem for me and doesn't affect the existing tests. I had a brief attempt at writing specs to prove this further, but it would require writing a suite for EM Synchrony which is possibly more effort than its worth at the moment.

@jcf
Collaborator
jcf commented May 26, 2011

I'm apprehensive about merging this request without tests. If there's no test coverage this could end up getting removed in a future release or break if and when EM changes.

I'm thinking we can add a unit test that calls #setup passing an error and simply assert that EM.next_tick is called. I'm not sure if this is good enough however.

describe '#setup' do
  context 'when an error occurs' do
    it 'uses next_tick to prevent a double resume FiberError' do
      # …
      EM.should_receive(:next_tick)
      setup(response, uri, "WebMock timeout error")
    end
  end
end
@JonRowe
JonRowe commented May 26, 2011

Thats a fair criticism, I didn't want to modify the existing EventMachine specs seeing as I doubt the problem exists in the same fashion for vanilla EM.

I'll look at deciphering what the web mock shared examples are doing (they don't just drop into synchrony) and write a spec suite for synchrony at some point. Then there'll be the added bonus of webmock being able to claim synchrony support ;)

@jcf
Collaborator
jcf commented May 26, 2011

That would be amazing! I might be able to spend some time on this over the weekend. I need to dedicate a bit of time to getting us down to Pull Requests Zero.
On Thursday, 26 May 2011 at 10:55, JonRowe wrote:

Thats a fair criticism, I didn't want to modify the existing EventMachine specs seeing as I doubt the problem exists in the same fashion for vanilla EM.

I'll look at deciphering what the web mock shared examples are doing (they don't just drop into synchrony) and write a spec suite for synchrony at some point. Then there'll be the added bonus of webmock being able to claim synchrony support ;)

Reply to this email directly or view it on GitHub:
#95 (comment)

@sdhull
sdhull commented Jun 13, 2011

FWIW, I still got double resume errors even with EM.next_tick being called in WebMock. I actually had to basically re-do how em-synchrony defines its callbacks to call EM.next_tick there. You can see it in my pull request here or that specific commit.

Cheers!

@bblimke
Owner
bblimke commented Jul 29, 2011

Closing this one because @sdhull fixed the issue and submitted complete pull request 111, together with specs.
JonRowe thank you for the patch anyway!

@bblimke bblimke closed this Jul 29, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment