If `env` is duped or otherwise not the same as the original `env` that was
generated at the top of rack middleware, it is impossible for the server hijack
proc to update the right `env` instance. Therefore, capturing the return value
is more reliable. This is the recommendation of the rack SPEC.
Remove all `include ActiveSupport::Testing::MethodCallAssertions`
in actioncable's tests since we can do it only in `ActionCable::TestCase`
in order to prevent code duplication.
We use the same approach for other modules of Rails.
We have defined `ActionCable::TestCase` in `actioncable/test/test_helper.rb`
that we can use in order to prevent code duplication and build common
interface for actioncable's test.
* Global ignores at toplevel .gitignore
* Component-specific ignores in each toplevel directory
* Remove `actionview/test/tmp/.keep` for JRuby
```
rm actionview/test/tmp/ -fr
cd actionview/
bundle exec jruby -Itest test/template/digestor_test.rb
```
Related to #11743, #30392.
Closes#29978.
### Summary
This PR changes .rubocop.yml.
Regarding the code using `if ... else ... end`, I think the coding style
that Rails expects is as follows.
```ruby
var = if cond
a
else
b
end
```
However, the current .rubocop.yml setting does not offense for the
following code.
```ruby
var = if cond
a
else
b
end
```
I think that the above code expects offense to be warned.
Moreover, the layout by autocorrect is unnatural.
```ruby
var = if cond
a
else
b
end
```
This PR adds a setting to .rubocop.yml to make an offense warning and
autocorrect as expected by the coding style.
And this change also fixes `case ... when ... end` together.
Also this PR itself is an example that arranges the layout using
`rubocop -a`.
### Other Information
Autocorrect of `Lint/EndAlignment` cop is `false` by default.
https://github.com/bbatsov/rubocop/blob/v0.51.0/config/default.yml#L1443
This PR changes this value to `true`.
Also this PR has changed it together as it is necessary to enable
`Layout/ElseAlignment` cop to make this behavior.
pg-1.0.0 is just released and most Gemfiles don't restrict
it's version. But the version is checked when connecting to
the database, which leads to the following error:
Gem::LoadError: can't activate pg (~> 0.18), already activated pg-1.0.0
See also this pg issue:
https://bitbucket.org/ged/ruby-pg/issues/270/pg-100-x64-mingw32-rails-server-not-start
Preparation for pg-1.0 was done in commit f28a331023fab,
but the pg version constraint was not yet relaxed.
I often face the following error when running test/connection/client_socket_test.rb:
```
$ bin/test test/connection/client_socket_test.rb:38
Run options: --seed 44035
# Running:
F
Failure:
ActionCable::Connection::ClientSocketTest#test_delegate_socket_errors_to_on_error_handler
[/app/actioncable/test/connection/client_socket_test.rb:47]:
--- expected
+++ actual
@@ -1 +1 @@
-["foo"]
+["Broken pipe", "Broken pipe", "Broken pipe", "foo"]
```
It can be reproduced easily by applying the following patch:
```
diff --git a/actioncable/test/connection/client_socket_test.rb b/actioncable/test/connection/client_socket_test.rb
index 2051216010..6bb9f13ea7 100644
--- a/actioncable/test/connection/client_socket_test.rb
+++ b/actioncable/test/connection/client_socket_test.rb
@@ -34,7 +34,8 @@ def on_error(message)
@server.config.allowed_request_origins = %w( http://rubyonrails.com )
end
- test "delegate socket errors to on_error handler" do
+ 1000.times do |i|
+ test "delegate socket errors to on_error handler #{i}" do
run_in_eventmachine do
connection = open_connection
@@ -47,6 +48,7 @@ def on_error(message)
assert_equal %w[ foo ], connection.errors
end
end
+ end
test "closes hijacked i/o socket at shutdown" do
run_in_eventmachine do
```
The cause is writing io from different thread at the same time.
`connection.process` sends handshake message from [StreamEventLoop's thread][] whereas
`connection.handle_open` sends welcome message from current thread.
[StreamEventLoop's thread]: 067fc779c4/actioncable/lib/action_cable/connection/stream_event_loop.rb (L75)
Simplify our dev testing and CI story since we're also testing against
Redis for the Active Support cache store.
Directly test whether db, host, password, etc are passed through as
config instead of spinning up a Redis server with a password set on it.
* Use `gem 'redis', '~> 4.0'` for new app Gemfiles
* Loosen Action Cable redis-rb dep to `>= 3.3, < 5`
* Bump redis-namespace for looser Redis version dep
* Avoid using the underlying `redis.client` directly
* Use `Redis.new` instead of `Redis.connect`
* When the adapter is missing, raise an exception that points out config
typos and missing Gemfile entries. (We can assume that a non-builtin
adapter was used since these are always available.)
* When loading an adapter raises a LoadError, prefix its error message
to indicate that the adapter is likely missing an optional dependency.
If a frontend for some reason tries to unsubscribe from a non existing subscription, the following error is logged:
Could not execute command from ({"command"=>"unsubscribe", "identifier"=>"{\"channel\":\"SomeChannel\"}"}) [NoMethodError - undefined method `unsubscribe_from_channel' for nil:NilClass]
Instead, it will now properly log:
Could not execute command from ({"command"=>"unsubscribe", "identifier"=>"{\"channel\":\"SomeChannel\"}"}) [RuntimeError - Unable to find subscription with identifier: {"channel":"SomeChannel"}]
Don't use remove_method or remove_possible_method just before a new
definition: at best the purpose is unclear, and at worst it creates a
race condition.
Instead, prefer redefine_method when practical, and
silence_redefinition_of_method otherwise.
In some examples and guides we are recommending to use code like:
```ruby
verified_user = User.find_by(id: cookies.signed[:user_id])
```
My suggestion is to use instead:
```ruby
verified_user = User.find_by(id: cookies.encrypted[:user_id])
```
which invites users to prefer the "newer" encrypted cookies over the
"legacy" signed cookies.
- Adds RedisAdapterTest::AlternateConfiguration to account
for a relatively common alternative setup, as it’s used
as the first example in the
[Redis rubygem](https://github.com/redis/redis-rb#getting-started)
- Supplies original RedisAdapterTest with more complete
redis:// url format by adding a ‘userinfo’ (blank user),
so that it resembles the alternate configuration
- Supplies original EventedRedisAdapterTest with more complete
redis:// url as well
- Adds before_script to start redis-server with password as a daemon
and with explicit defaults copied from the default redis.conf
(Instead of using Travis' default init/upstart scripts for `redis` service)
* Allow a default value to be declared for class_attribute
* Convert to using class_attribute default rather than explicit setter
* Removed instance_accessor option by mistake
* False is a valid default value
* Documentation
Before this commit, the database connection used in Action Cable's
PostgreSQL adapter was "owned" by `ActiveRecord::Base.connection_pool`.
This meant that if, for example, `#clear_reloadable_connections!` was called on the pool, Active
Record would "steal" the database connection from Action Cable, and
would cause all sorts of issues. This became evident during file
reloads; despite Action Cable trying its hardest to return its borrowed
database connection to Active Record via `@pubsub.shutdown`, Active Record calls
`#clear_reloadable_connections!` on the connection pool, and due to the order of callbacks, Active
Record's callback was being executed first. This meant that if you tried
to rerender a view after a file was reloaded, you would have to wait
through Active Record's timeout and such.
Now, Action Cable takes direct ownership of the database connection it
uses. It removes the connection from the pool to avoid the situation
described above. Action Cable also makes sure to call `#disconnect!` on
the connection when appropriate, to match the previous behavior of
Active Record.
[ Jon Moss & Matthew Draper]
This reverts commit 296d024b4e91c4891ae0b010249193513e63b921, reversing
changes made to e341d835070c7ef9990f41e02bbf46536be0aee7.
We aren't trying to compare to current_user, we're assigning that variable.
According to the commit comment on 54d84cbb77a7fbc6359eda4eb40cc7da29c426e1, AJ/bin/test was intentionally not added,
but AJ tests doesn't actually do anything special other than specifying ENV['AJ_ADAPTER'],
which can be easily done via command line environment variable.
`assert_raise` does not check error message. However, in some tests,
it seems like expecting error message checking with `assert_raise`.
Instead of specifying an error message in `assert_raise`, modify to use
another assert to check the error message.
ActionCable was throwing a "Existing connection must be closed before
opening" exception which was being picked up as a production issue in
our error monitoring software. Since this happens pretty often on any
device that allows the browser to sleep (mobile) this error was getting
triggered often.
This change removes the exception, but keeps logging the occurrence. We
now return `false` to let the caller now that `open` failed.
nio4r 2.0.0 primarily includes new features and bugfixes, with few breaking
changes. The primary reason for bumping the major version is dropping support
for all Ruby versions prior to 2.2.2, so as to match Rails 5.
Full release announcement here:
https://groups.google.com/forum/#!topic/socketry/ZDIUj1ufiJ8
reactor_running? will be true just after the thread enters
EventMachine.run; reactor_thread only gets set after the internal
initialize_event_machine method has been called, the C extension is set
up, and it is entering its run loop.
WebSocket always defers the decision to the server, because it didn't
have to deal with legacy compatibility... but the same-origin policy is
still a reasonable default.
Origin checks do not protect against a directly connecting attacker --
they can lie about their host, but can also lie about their origin.
Origin checks protect against a connection from 3rd-party controlled
script in a context where a victim browser's cookies will be passed
along. And if an attacker has breached that protection, they've already
compromised the HTTP session, so treating the WebSocket connection in
the same way seems reasonable.
In case this logic proves incorrect (or anyone just wants to be more
paranoid), we retain a config option to disable it.
IO#close and IO#read across threads don't get along so well:
After T1 enters #read and releases the GVL, T2 can call #close on the
IO, thereby both closing the fd and freeing the buffer while T1 is using
them.
Before this patch, if you were to make a file edit in your Rails
application and you tried to load up the page, it would hang
indefinitely. The issue is that Active Record is trying to cleanup after
itself and clear all active connection, but Action Cable is still
holding onto a connection from the pool. To resolve this, we are now
shutting down the pubsub adapter before classes are reloaded, to avoid
this altogether (connection is being returned to the pool).
Credits to @skateman for discovering this bug. :)
Mostly, this is just to avoid EventMachine. But there's also an argument
to be made that we're better off using a different protocol library for
our test suite than the one we use to implement the server.
When the `allow_same_origin_as_host` is set to `true`, the request
forgery protection permits `HTTP_ORIGIN` values starting with the
corresponding `proto://` prefix followed by `HTTP_HOST`. This way
it is not required to specify the list of allowed URLs.
Fixes#23757.
Before this commit, even if `reject` was called in the `subscribe`
method for an Action Cable channel, all actions on that channel could
still be invoked. This calls a `return` if a rejected connection tries
to invoke any actions on the channel.
Inserted spaces in the name of Rails components.
Since I was on it, also used PostgreSQL instead of Postgres
because albeit Postgres is an accepted alias, PostgreSQL is
the official name and the actual name of the adapter.
See
https://wiki.postgresql.org/wiki/ProjectName
with regard to PostgreSQL vs Postgres.
- Remove "Configuration", "Running the server", "Dependencies" and "Deployment"
sections from the Action Cable README as they are already duplicated in the
Action Cable overview guide.
Apps that depend on Action Cable don't need Blade for app development,
so we can remove the gem dependency.
We do need Blade for Action Cable dev, so we bundle it in the Gemfile.
Signed-off-by: Jeremy Daer <jeremydaer@gmail.com>
- Now we will detect what javascript engine user is using and based on
that we will generate either `.js` or `.coffee` version of the channel
file.
- This also needs a change in coffee-rails to override the `js_template`
method. Related PR https://github.com/rails/coffee-rails/pull/72.
- Currently coffee-rails gem sets
`config.app_generators.javascript_engine` to `:coffee` and using this
information we override the `js_template` to set the extension as
`.coffee` in coffee-rails gem.
- Using this approach, we can keep the `channel.js` and `channel.coffee`
files in the Rails repository itself.
- Additionally the `js_template` method can act as public interface for
coffee-rails gem to hook into and change the extension to `.coffee`
without maintaining the actual asset files.
[Prathamesh Sonpatki, Matthew Draper]
