-
Notifications
You must be signed in to change notification settings - Fork 22
Reduce the scope of the illegal call tracer and other tracing fixes #332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Converting to draft while we investigate why |
b77f223 to
6ebf37e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These utilities are here just to help display/debug GC issues if there is a failure in the GC test
6ebf37e to
846e5d5
Compare
| parent = {} # child_id -> parent_id | ||
| root_of = {} # obj_id -> root_category | ||
|
|
||
| roots.each do |category, objs| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't believe you had to write a graph search lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI did it, not me 😁. I'd never devote this kind of time to a test utility only invoked on failure, but I figured since I used it and confirmed it's a good utility, I might as well keep it around and use it on failure. I'd also never write such difficult-to-comprehend code, I'd have more human friendliness in lots of aspects.
| end | ||
| end | ||
|
|
||
| def test_many_children |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this trigger the detector before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In slower machines like some CI runners, yes. In faster machines, COUNT needs to be 1000.
|
Ug, getting GC fail on 3.4 sometimes, and getting server-caused hanging on repeated |
177d7d5 to
20c59ba
Compare
20c59ba to
3660e90
Compare
|
I will be handling GC issues separately in #334. I skipped the test and this PR is now good, will merge once CI done. |
* envconfig impl * linting * fix steep type checks * formatting * Refactor envconfig classes to use Data.define for immutability - Refactor ClientConfigTLS, ClientConfigProfile, and ClientConfig to use Data.define * Rename hash methods to use idiomatic Ruby naming - Rename from_hash → from_h for all envconfig classes - Rename to_hash → to_h for all envconfig classes * Replace TLS config hash with Connection::TLSOptions object - Rename `to_connect_tls_config` → `to_tls_options` - Return `Connection::TLSOptions` object instead of plain Hash * Refactor to use inline pattern instead of temporary variables * Refactor to_client_connect_options to return tuple for one-liner usage Changed `to_client_connect_options` to return `[positional_args, keyword_args]` tuple instead of a hash, enabling clean one-liner client connections: ```ruby args, kwargs = profile.to_client_connect_options client = Temporalio::Client.connect(*args, **kwargs) ``` * Make hash methods private * Remove File.exist? check by using type system for path vs data distinction - TOML paths (*_path fields) become Pathname objects - TOML data (*_data fields) remain String objects * Use symbol keys instead of string keys in Rust FFI bridge Changed all hash keys from strings to symbols in the Rust-to-Ruby bridge * Bump golang.org/x/net in /temporalio/test/golangworker (#303) * Bump tracing-subscriber from 0.3.19 to 0.3.20 in /temporalio (#331) * Reduce the scope of the illegal call tracer and other tracing fixes (#332) * Remove unnecessary fully qualified import names * Remove envconfig import in temporalio.rb * _source_to_path_and_data prefixed with underscore and private doc visibility * TLS disabled field now optional * Rubocop fixes * Steep fixes * Renamed envconfig.rb -> env_config.rb Renamed to_tls_options -> to_client_tls_options * Pass data string as RString, Rust bridge converts to Vec * Rename load_client_connect_config -> load_client_connect_options * Format envconfig.rs * Nits * Update core submodule * Revert irrelevant cancellation changes * Update Cargo.lock * Propagate set_headers failure in client bridge * Add experimental warning --------- Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chad Retz <chad.retz@gmail.com>
What was changed
Deadlock detector was tripping on 1000 child workflows at once (see #330). This was because our illegal call tracer can be expensive. We made the following improvements:
run_until_all_yieldedcode which allows us to save many calls to theTracePointTracePoint#disablewhich was showing issues of not being reentrantworkflow_instance.rbto no longer delegate two methods deep to apply activation (helps stack traces and code legibility)Checklist