Patch EventMachine and Thin to call existing trap handlers

[christianprescott]

This change monkey patches some dependencies to call any existing handlers of trap signals when registering their own.

EventMachine, Thin, and rspec all register their own handlers for the INT signal. (among others) None of them consider existing handlers so they end up disabling behavior in the other libraries. I think PusherFake is where this change belongs since it's the intersection of the three?

Motivation

The specific problem I'm trying to solve is these libraries swallowing up an INT signal that should be received by rspec. Typically interrupting rspec allows it to finish the current test and gracefully shut down:

.........^C
RSpec is shutting down and will print the summary report... Interrupt again to force quit.
.

Finished in ...

But with PusherFake's server running the interrupt is never received by rspec at all, and the user has to kill the process or wait for the tests to complete on their own:

.......^CTerminating WebSocket Server
......^CTerminating WebSocket Server
F..........^CTerminating WebSocket Server
F......

Failures: ...

[tristandunn]

Thanks for the PR! (You actually submitted it on my birthday, so bit of a delay getting back.) This has been around for a long time but I never looked into it due to the relatively small tests I run with pusher-fake.

I did some research around this and came to the same conclusion as you that it should chain the trap handlers for it function properly, unfortunately this patch doesn't actually work for me either. Can you verify it's working for you?

Ended up debugging it a bunch, so here's a dump of what I figured out:

• Messing with a version locally and it seems I can get it working if I patch the libraries immediately after including them and before defining the PusherFake module.
• I believe it's possible the traps have already been added by the time the server is loaded, likely from the spec/support/webhooks.rb code starting a Thin server to handle webhooks.
• Which I'm guessing likely means it'd work for you in a project without that, but not in this test suite.
• It looks like it works if I call it before that! Amazing.

I just spent way too much time figuring that out, but I'll get your commit patched up this week and get it in. Mostly because I'd like to isolate the calls to within the libraries module or test suite, instead of globally. And I might consider adding an option for disabling it, but not sure why anyone would want to do that. Apart from the entire trap chain being executed being a very unexpected behavior in Ruby.

[tristandunn]

Here's what I came up with: https://github.com/tristandunn/pusher-fake/compare/td-chain-trap-handlers

I duplicated the code for the test suite here instead of making the method public, which is the only thing I didn't like during my last comment. I'll work on getting this released in the next few days and make sure you get credit. Thanks so much!

[tristandunn]

Argh, this still doesn't quite work for this test suite since I start a server in a thread for every feature... I'll have to work on avoiding that or figure out another fix.

[christianprescott]

:birthday: Happy birthday! No need to explain. :birthday:

It does work in my project's test suite. I think your assessment is correct - the patch needs to be in place at server start time, so makes sense that spec/support/webhooks.rb would have that effect. Good find. I don't see any harm in applying the patch even earlier. Or yeah the duplication strategy works as well.

• Do you think it's necessary to add a block_given? condition to the trap method? All the usages we're concerned with pass a block, but technically trap accepts a string argument for the command. I haven't thought through what would happen if such an argument were used.
• When I apply your change a LoadError is raised for pusher-fake/server/chain_trap_handlers, but if it works for you I'm probably doing something wrong.

[tristandunn]

Thanks!

Do you think it's necessary to add a block_given? condition to the trap method?

Good call, will do.

When I apply your change a LoadError is raised [...]

Hmm, maybe I pushed a bad change. I'll check and test in my pusher-fake-example project.

[tristandunn]

@christianprescott Opened https://github.com/tristandunn/pusher-fake/pull/75, but we can keep discussing here if you'd like. It seems to work fine in my example project, would you mind giving it a try?

gem "pusher-fake", github: "tristandunn/pusher-fake", branch: "td-patch-trap"

[tristandunn]

Closing in favor of https://github.com/tristandunn/pusher-fake/pull/75.