Updates `MemoryStore#write_entry` to pass a `nil` `namespace` to
`exist?`, which expects a _name_ rather than a an already "normalized"
_key_. This fixes a bug where `unless_exist` would overwrite any
existing entry if a `namespace` was used.
Right now if the subprocess exit uncleanly, it straight out bring the
parent down with it because it will fail to parse the (likely empty)
Marshal payload:
```
<internal:marshal>:34:in `load': marshal data too short (ArgumentError)
from 3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:23:in `run'
from 3.3.0+0/gems/minitest-5.20.0/lib/minitest.rb:1094:in `run_one_method'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:179:in `block in run'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:168:in `with_timestamps'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:178:in `run'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:229:in `block in run_from_queue'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/ci/queue/redis/worker.rb:55:in `poll'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:228:in `run_from_queue'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:213:in `__run'
from 3.3.0+0/gems/minitest-5.20.0/lib/minitest.rb:162:in `run'
from 3.3.0+0/gems/minitest-5.20.0/lib/minitest.rb:86:in `block in autorun'
- XXXX::XXXXTest#test_xxxxx - /tmp/bundle/ruby/3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:52:in `write': closed stream (IOError)
from 3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:52:in `puts'
from 3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:52:in `block (2 levels) in run_in_isolation'
from 3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:32:in `fork'
from 3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:32:in `block in run_in_isolation'
from 3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:28:in `pipe'
from 3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:28:in `run_in_isolation'
from 3.3.0+0/bundler/gems/rails-488a7ce18880/activesupport/lib/active_support/testing/isolation.rb:19:in `run'
from 3.3.0+0/gems/minitest-5.20.0/lib/minitest.rb:1094:in `run_one_method'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:179:in `block in run'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:168:in `with_timestamps'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:178:in `run'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:229:in `block in run_from_queue'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/ci/queue/redis/worker.rb:55:in `poll'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:228:in `run_from_queue'
from 3.3.0+0/gems/ci-queue-0.38.0/lib/minitest/queue.rb:213:in `__run'
from 3.3.0+0/gems/minitest-5.20.0/lib/minitest.rb:162:in `run'
from 3.3.0+0/gems/minitest-5.20.0/lib/minitest.rb:86:in `block in autorun'
```
This breaks the Minitest contract that `run_one_method` shouldn't raise
ever, and return a `Minitest::Result`.
By properly checking the sub process status, we can turn this crash into
a test failure, allowing the original test process to go on.
It's a common useful pattern for situation where something isn't
supposed to happen, but if it does we can recover from it.
So in such situation you don't want such issue to be hidden
in development or test, as it's likely a bug, but do not want to
fail a request if it happens in production.
In other words, it behaves like `#record` in development and test
environments, and like `raise` in production.
Fix: https://github.com/rails/rails/pull/49638
Fix: https://github.com/rails/rails/pull/49339
Co-Authored-By: Andrew Novoselac <andrew.novoselac@shopify.com>
Co-Authored-By: Dustin Brown <dbrown9@gmail.com>
The return value was not specified before. Now it returns `true` on a
successful write, `nil` if there was an error talking to the cache
backend, and `false` if the write failed for another reason (e.g. the
key already exists and `unless_exist: true` was passed).
Fix: #50047
`Backtrace::Location` instance for code generated with eval always
have their `absolute_path` set to `nil`. So if absolute path is nil
we should fallback to checking `#path`.
Co-Authored-By: fatkodima <fatkodima123@gmail.com>
Previously, the `Cache::Store` instrumentation would call
`normalize_key` when adding a key to the log. However, this resulted in
the logged key not always matching the actual key written/read from the
cache:
```irb
irb(main):004> cache.write("foo", "bar", namespace: "baz")
D, [2023-11-10T12:44:59.286362 #169586] DEBUG -- : Cache write: baz:foo ({:compress=>false, :compress_threshold=>1024, :namespace=>"baz"})
=> true
irb(main):005> cache.delete("foo", namespace: "baz")
D, [2023-11-10T12:45:03.071300 #169586] DEBUG -- : Cache delete: foo
=> true
```
In this example, `#write` would correctly log that the key written to
was `baz:foo` because the `namespace` option would be passed to the
`instrument` method. However, other methods like `#delete` would log
that the `foo` key was deleted because the `namespace` option was _not_
passed to `instrument`.
This commit fixes the issue by making the caller responsible for passing
the correct key to `#instrument`. This allows `normalize_key` to be
removed from the log generation which both prevents the key from being
normalized a second time and removes the need to pass the full options
hash into `#instrument`.
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
On the first call it can be a bit slow because it needs to load
a bunch of data. It's also better to do it as part of boot
so that some of that data is shared via Copy-on-Write
Follow up to previous commit where these procs were allows to be shared
between subclasses. This change allocates the procs as they are first
declared.
Because `#present?` always resolve to `Object#present?`, it's
an extremely polymorphic method, and inline cache hits are low.
In addition, it requires an extra call to `self.blank?` which is
an overhead.
By specializing `present?` on common types, we avoid both of these
slow-downs:
```
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin22]
Warming up --------------------------------------
present? 198.028k i/100ms
opt_present? 565.521k i/100ms
Calculating -------------------------------------
present? 2.087M (± 8.8%) i/s - 10.297M in 5.028398s
opt_present? 5.584M (± 8.6%) i/s - 27.711M in 5.023852s
Comparison:
present?: 2086621.6 i/s
opt_present?: 5584373.5 i/s - 2.68x faster
```
```
ruby 3.2.2 (2023-03-30 revision e51014f9c0) +YJIT [arm64-darwin22]
Warming up --------------------------------------
present? 819.792k i/100ms
opt_present? 1.047M i/100ms
Calculating -------------------------------------
present? 12.192M (± 8.8%) i/s - 60.665M in 5.050622s
opt_present? 16.540M (± 8.2%) i/s - 82.676M in 5.059029s
Comparison:
present?: 12192047.5 i/s
opt_present?: 16539689.6 i/s - 1.36x faster
```
```ruby
require 'bundler/inline'
gemfile do
source 'https://rubygems.org'
gem 'benchmark-ips'
gem 'activesupport'
end
require 'active_support/all'
class Object
def opt_present?
respond_to?(:empty?) ? !empty? : !!self
end
end
class NilClass
def opt_present?
false
end
end
class FalseClass
def opt_present?
false
end
end
class TrueClass
def opt_present?
true
end
end
class Array
def opt_present?
!empty?
end
end
class Hash
def opt_present?
!empty?
end
end
class Symbol
def opt_present?
!empty?
end
end
class String
def opt_present?
!blank?
end
end
class Numeric # :nodoc:
def opt_present?
true
end
end
class Time # :nodoc:
def opt_present?
true
end
end
array = []
hash = {}
time = Time.now
puts RUBY_DESCRIPTION
Benchmark.ips do |x|
x.report("present?") do
true.present?
false.present?
1.present?
1.0.present?
array.present?
hash.present?
:foo.present?
time.present?
end
x.report("opt_present?") do
true.opt_present?
false.opt_present?
1.opt_present?
1.0.opt_present?
array.opt_present?
hash.opt_present?
:foo.opt_present?
time.opt_present?
end
x.compare!(order: :baseline)
end
```
The `@` prefix is always stripped, so might as well not require it.
For backward compatibility reasons we still handle the prefix for now,
but we eagerly strip it and emit a deprecation.
Prior to this commit, `String#downcase_first` and `String#upcase_first`
would return a frozen string when called on an empty string:
```ruby
# BEFORE
"foo".downcase_first.frozen? # => false
"".downcase_first.frozen? # => true
# AFTER
"foo".downcase_first.frozen? # => false
"".downcase_first.frozen? # => false
```
For better or worse, the Rails guide settled on double quotes
and a large part of the community also use rubocop which enforce
them by default.
So we might as well try to follow that style when providing code
snippets in the documentation or error messages.
Fix: https://github.com/rails/rails/issues/49822
I certainly didn't get them all, but consistency should be significantly
improved.
For SEO purposes, there should only be one `<h1>` per page, and the edge
version of SDoc will provide one using the module name if none are
present in documentation.
The reason of removing dup 5 years ago in 6da99b4e99 was to decrease memory allocations, so, it makes sense to only dup options when we know they will be overwritten.
We had a few cases of tests being skipped accidentally on CI
hence not bein ran for a long time.
Skipping make sense when running the test suite locally, e.g.
you may not have Redis or some other dependency running.
But on CI, a test not being ran should be considered an error.
Previously in https://github.com/rails/rails/pull/43282, which shipped
with Rails 7.0, we added the guarantee that if an exception occurred
within an ActiveSupport::Notificaitons subscriber that the remaining
subscribers would still be run.
This was broken in https://github.com/rails/rails/pull/44469, where the
different types of subscribers were broken into groups by type for
performance. Although we would guard exceptions and allways run all (or
none) of the same group, that didn't work cross-group with different
types of subscriber.
Any classes that don't implement #blank? will fall back to Object's
implementation, which checks whether #empty? is defined before either
using #empty? or implicit truthiness. Since checking whether #empty? is
defined is expensive, some core classes (Array/Hash) alias #blank?
directly to #empty? to skip the respond_to? check.
This commit applies the alias optimization to Symbol. #blank? is called
on Symbols for many Active Record query methods (select, includes,
group, order, joins, etc.) so it seems reasonable to define the #blank?
alias on Symbol as well.
`with_test.rb` should define a class named `WithTest`. Instead, it
redefines `BlankTest`. This seems to be a copy/paste typo.
This commit renames `BlankTest` to `WithTest` to follow convention, and
to avoid any potential conflicts of 2 test files defining the same
class.
ActiveSupport::Callbacks often ended up with empty Arrays: on callbacks
without a conditional, and on callback sequences which had no before
and/or after callbacks.
Previously, callbacks which were shared with subclasses would not share
between them the procs generated in Filters::Before and Filters::After.
This was also lazily generated so would cause memory growth after an
application is booted (and not sharable between workers via CoW).
This was because we would rebuild the objects used to invoke the
callbacks via CallbackChain#compile, so any difference in the callback
chain would result in all of the callback procs being rebuilt.
This commit changes before and after callbacks (but not around!) to be
shared between all subclasses of where it was defined. This is done by
changing Filters::Before and Filters::After to plain classes which
respond to call instead of generating procs (which wasn't strictly
necessary but was easier to implement, and also results in simpler
objects which use less memory). These objects avoid referencing and tied
to a specific callback sequence and so can be memoized and reused.
This has the most impact on applications with many Controllers, and many
callbacks in the ApplicationController (or similar).
I also took this opportunity to merge together all the different forms
of procs generated (halting, halting_and_conditional, conditional,
simple) into one form with if statements. There isn't any significant
performance benefit from the specialization previously being done.
In `bootstrap.rb` we set the `Rails.logger.level` to `config.log_level`. But at this point, we may have already set up a `BroadcastLogger` with multiple broadcasts that have different levels. So, calling `level=` on the `BroadcastLogger` will overwrite the level of the individual broadcasts. So instead, let's only set the `Rails.logger.level` if the logger is not a `BroadcastLogger`.
It used to be stdlib but is being extracted in modern rubies.
Overall its usefulness is dubious. In all cases it is included in
Rails, it's only for the `synchronize` method, but end up exposing
a dozen other useless methods.
In the end just using a Mutex is clearer and simpler.
In some cases we can even get away with a single mutex in a constant.
