Eliminate the EventMachine dependency #23152
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👏 |
| return true if env['rack.url_scheme'] == 'https' | ||
|
|
||
| return false | ||
| end |
There was a problem hiding this comment.
We can utilise Rack::Request#ssl? for this can't we?
There was a problem hiding this comment.
Indeed.. I'll look at chasing env -> req right through in a subsequent change, because I imagine that'll make @tenderlove happy.
|
@matthewd 👍 - does this mean we could run it all in one server in development mode? |
|
@matthewd 👍 Does this mean there's work to be done on reloading? |
matthewd
force-pushed
the
actioncable-concurrent
branch
2 times, most recently
from
07fbf3f
to
65e3232
Compare
| headers.each { |name, value| @driver.set_header(name, value) } | ||
| end | ||
|
|
||
| [*options[:extensions]].each do |extension| |
There was a problem hiding this comment.
why use two different array handling styles between :headers and :extensions?
matthewd
force-pushed
the
actioncable-concurrent
branch
2 times, most recently
from
070ada8
to
73bf1a5
Compare
| [ -1, {}, [] ] | ||
| end | ||
|
|
||
|
|
matthewd
force-pushed
the
actioncable-concurrent
branch
from
73bf1a5
to
438587d
Compare
(as adapted to use concurrent-ruby / nio4r instead of eventmachine)
We're no longer doing our work in the EM event loop, so errors are quite unlikely, and if they do occur, they're not really our responsibility to handle.
matthewd
force-pushed
the
actioncable-concurrent
branch
from
438587d
to
503fe75
Compare
dhh
added a commit
that referenced
this pull request
Jan 24, 2016
Eliminate the EventMachine dependency
| @todo = Queue.new | ||
|
|
||
| Thread.new do | ||
| Thread.current.abort_on_exception = true |
There was a problem hiding this comment.
You probably don't want this.
claudiob
added a commit
to claudiob/rails
that referenced
this pull request
Jan 26, 2016
Since rails#23152 eliminated the EventMachine dependency, we don’t need to explicitly mention EventMachine. Nevertheless, I'm not 100% sure about saying "the websocket-driver loop" driver… any suggestions, @matthewd or @pixeltrix ? 😅 [ci skip]
|
Testing this with Basecamp, we're getting two separate exceptions. First: Which then leads to: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@pixeltrix pointed out that it wasn't actually all that infeasible for us to just import the portion of faye-websocket we need... it turns out to only be about 200 lines. So I did that.
All the protocol complexities live in websocket-driver, which we're still using, so we're not taking on any substantial maintenance burden.
For this relatively low cost, I'd contemplate this just to reduce our dependency list by the one gem... but the important part here is that by importing it, we're able to (as I have) then modify it to avoid EventMachine.