Ruby GC crash in ObjectSpace::WeakKeyMap #3
Closed
Conversation
This file contains hidden or 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
Fix: rails#52429 The previous implementation assumed `NoMethodError` would be raised when calling `super`, but that's not always true. If the receiver is an implicit self, the raised error will be `NameError`. It's better not to rely on exceptions for this anyways.
Fix `delegate_missing_to allow_nil: true` when called with implict self
Adapters have very inconsistent internal APIs to perform queries. This refactoring tries to improve consistency with a common provite API for all of them. Abstract methods: - `raw_execute`: the only method where an adapter should perform a query. It returns a native, adapter specific result object. Does not apply query transformations. Does not check for writes. - `cast_result`: receives the native result object and returns a generic `ActiveRecord::Result`. - `affected_rows`: receives the native result object and returns the number of affected rows. By just implementing these 3 methods all adapters automatically get: - `raw_exec_query`: same as `raw_execute` but returns an `ActiveRecord::Result`. - `internal_exec_query`: same as `raw_exec_query` but check for writes and apply query transformations. - `internal_execute`: same as `internal_exec_query` but retuns the native, adapter specific, result object. With this increased conisistency, we can now reduce the ammount of duplicated code in every adapter. There's some room for futher improvments but I tried to not go too far all at once. Also previously some adapters had a block based query interface that allowed to eagerly clear the native result object. It may make sense to bring that capability back in a consistent way, but short term I opted for consistency.
The postgres adapter used to be more complex than the others to be able to pass the prepared statement key to the `log` method. If we instead add it to the payload later, we can simplify the method further and it opens the door to refactor `log` and `with_raw_connection` out of `raw_execute`.
A `raw_execute` implementation is now provided, instead adapters have to implement `perform_query`. It's a much simpler method that no longer need to concern itself with Active Support notifications nor calling `with_raw_connection`.
Inline simple operations like reseting the timezone.
Normalizes email_address before saving it to the DB.
and the sessions view template has been moved into an erb generator so that gems like tailwindcss-rails can provide a specialized template.
Not worth the complication. Let them login again. It doesn't matter.
It's slower than it needs to because it contains a workaround for
a bug affecting early Ruby 3.2 releases.
It also matches against a regexp which isn't necessary given
that starting in Ruby 3.2, `Time.new(str)` is strict enough
for this purpose.
```
ruby 3.3.3 (2024-06-12 revision f1c7b6f435) +YJIT [arm64-darwin23]
Warming up --------------------------------------
bugged 265.827k i/100ms
good 394.774k i/100ms
Calculating -------------------------------------
bugged 2.759M (± 5.8%) i/s - 13.823M in 5.031296s
good 4.128M (± 7.8%) i/s - 20.528M in 5.010538s
Comparison:
bugged: 2758945.6 i/s
good: 4128323.9 i/s - 1.50x faster
```
```ruby
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
gem "benchmark-ips"
end
require 'benchmark/ips'
ISO_DATETIME = /
\A
(\d{4})-(\d\d)-(\d\d)(?:T|\s) # 2020-06-20T
(\d\d):(\d\d):(\d\d)(?:\.(\d{1,6})\d*)? # 10:20:30.123456
(?:(Z(?=\z)|[+-]\d\d)(?::?(\d\d))?)? # +09:00
\z
/x
def old(string)
return unless ISO_DATETIME.match?(string)
::Time.at(::Time.new(string, in: "UTC"))
end
def fast(string)
return unless string.include?("-") # Time.new("1234") # => 1234-01-01 00:00:00
::Time.new(string, in: "UTC")
end
str = "2024-01-01T12:43:13"
Benchmark.ips do |x|
x.report("bugged") { old(str) }
x.report("good") { fast(str) }
x.compare!(order: :baseline)
end
```
Optimize `fast_string_to_time`
* Add bin/dev by default Then we have a uniform way of starting dev mode whether someone is using jsbundling or importmaps. * Also generated here
The docs usage should align with the method.
Here are the examples:
```
[29] pry(main)> ActiveSupport::JSON::Encoding.use_standard_json_time_format = true
=> true
[30] pry(main)> Time.utc(2005,2,1,15,15,10).in_time_zone("Hawaii").to_json
=> "\"2005-02-01T05:15:10.000-10:00\""
[31] pry(main)> Time.utc(2005,2,1,15,15,10).in_time_zone("Hawaii").as_json
=> "2005-02-01T05:15:10.000-10:00"
[32] pry(main)> ActiveSupport::JSON::Encoding.use_standard_json_time_format = false
=> false
[33] pry(main)> Time.utc(2005,2,1,15,15,10).in_time_zone("Hawaii").as_json
=> "2005/02/01 05:15:10 -1000"
[34] pry(main)> Time.utc(2005,2,1,15,15,10).in_time_zone("Hawaii").to_json
=> "\"2005/02/01 05:15:10 -1000\""
```
Support batching using custom columns
The payload construction was moved here in a previous commit, but log wasn't removed or deprecated. We've been using this as a hook we can override to add some adapter specific-keys. If we want to remove or bypass this at a later date we should probably introduce something that can be overridden like we did for cache_notification_info. Co-authored-by: Daniel Colson <danieljamescolson@gmail.com>
When config.i18n.raise_on_missing_translations = true, controllers and views raise an error on missing translations. However, models won't. This commit changes models to raise an error when raise_on_missing_translations is true Co-authored-by: Alex Ghiculescu <alex@tanda.co> Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
[ci skip] docs: use `as_json` per method instead of `to_json`
…tions Change ActiveModel human_attribute_name to raise an error
We special case singleton methods being added to ActiveRecord::Base subclasses whose names match methods on Kernel so that we can eagerly define them on the ClassSpecificRelation as otherwise we'd never hit method_missing. Previously, this would also eagerly define methods which were on the Kernel Module's singleton class rather than being defined as instance methods inside the module. Most notably this hit "name" which is defined on the anonymous HABTM classes.
If name is called on Relation it should be delegated to the model and doesn't require scoping.
…-structure-parsing Fix SQLite table definition parsing bug to handle commas in default function definitions
…message Mention which blob can't be transformed/previewed and what content_type it has. Co-authored-by: zzak <zzakscott@gmail.com>
Improve ActiveStorage::InvariableError message
…ails#52447) * lib/assets is too rare of a use case to warrant a default directory Either these assets are part of your app, and should be in app/assets, or you're getting them from a vendor, and they should be in vendor/assets. * Fix test
Edits to the scripts entry in CHANGELOG
Avoid calling Headers#env_name when static header
Split AR::Migration.load_schema_if_pending! into two methods
Minitest 5.25+ has a new API for `with_info_handler` that accepts an additional argument. The code now support all versions of minitest 5. See minitest/minitest@8cd3b1c
Support minitest 5.25+
…-extension Update PostgreSQLAdapter#extensions to include schema name
Converts hashes to keywords in ActionDispatch::Resources::Resource and in ActionDispatch::Scope to match syntax of other mapping methods and allocate less.
Removes racc dependency, and writes our own parser for routing instead.
…t_test Fixed failure in bin setup test
Fix: rails#52601 Now that Active Record connections are fully lazy, just calling `.lease_connection` isn't actually enough to estalish a connection. We might as well suggest to use a method with the actual intent.
…ssage Fix ActiveRecord::Base.inspect to correctly indicate how to load schema
Fix: rails#52615 Ref: rails#50796 The code assumed the application follow the standard `Name::Application` naming, but it's possible to rename it however you like.
rails console: Handle non standard application names
Use keywords with scopes and resources
…ions Allow to eager load nested nil associations
…nore Fix swallowing ignore order warning when batching using `BatchEnumerator`
Fix: rails#52617 Closes: rails#52618 It isn't per say a leak because the connection reaper eventually prune these, and also it's not expected that new Threads are constantly making entries into that cache. But it's true that with Fiber it's probably a bit more common, and the default reaper frequency isn't adapted to clear this fast enough. So we should instead eagerly prune it, or use a WeakMap. On 3.3+ we can use ObjectSpace::WeakKeyMap, but on older Ruby versions we have to resort to eager clearing.
|
One possibly related things, the keys in that weak map are either So perhaps that why, but I still haven't managed to reduce it. |
byroot
added a commit
that referenced
this pull request
Aug 16, 2024
Being investigated at #3 It seems that it's caused by using Thread or Fiber as keys.
|
Investigated with @peterzhu2118, and filed at https://bugs.ruby-lang.org/issues/20691 with a patch. |
DanielaVelasquez
pushed a commit
to DanielaVelasquez/rails
that referenced
this pull request
Oct 3, 2024
Being investigated at byroot#3 It seems that it's caused by using Thread or Fiber as keys.
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.
Opening here for now because I'm not able to reduce it. But with this Rails branch, I get Ruby to crash most of the time while running Active Record test suite.
It impact Ruby 3.3.4 and ruby-head.
The pointer seem to be garbage, so I wonder if perhaps it's because the hash was GCed prior to the key or something?
cc @peterzhu2118
I'll try to see if I can reduce it, but if you wish to have a look yourself, right now the repro is:
It crashes almost every time, if you run this a handful of time you should see the crash.