GH-50302: [GLib][Ruby][FlightRPC] Fix GC related problems#50401
Merged
Conversation
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the GLib Arrow Flight server binding to retain a reference to the GAFlightServerOptions passed to gaflight_server_listen(), preventing the options (and indirectly the auth handler object graph) from being freed while the server is still alive.
Changes:
- Store
GAFlightServerOptions*inGAFlightServerPrivateand keep it referenced across the server lifetime. - Add a
disposeimplementation to release the stored options reference when the server object is torn down. - Update
gaflight_server_listen()to replace the stored options reference on each listen.
Comment on lines
+1254
to
+1259
| auto priv = GAFLIGHT_SERVER_GET_PRIVATE(server); | ||
| if (priv->options) { | ||
| g_object_unref(priv->options); | ||
| } | ||
| priv->options = options; | ||
| g_object_ref(priv->options); |
3b0771c to
3df3a78
Compare
3df3a78 to
8f41f6b
Compare
Comment on lines
+1205
to
+1208
| if (priv->options) { | ||
| g_object_unref(priv->options); | ||
| priv->options = nullptr; | ||
| } |
Comment on lines
+1254
to
+1261
| auto priv = GAFLIGHT_SERVER_GET_PRIVATE(server); | ||
| if (priv->options != options) { | ||
| g_object_ref(priv->options); | ||
| if (priv->options) { | ||
| g_object_unref(priv->options); | ||
| } | ||
| priv->options = options; | ||
| } |
8f41f6b to
853822b
Compare
853822b to
8239181
Compare
Comment on lines
+1205
to
+1209
| if (priv->options) { | ||
| g_object_unref(priv->options); | ||
| priv->options = nullptr; | ||
| } | ||
|
|
Comment on lines
136
to
139
| macos: | ||
| name: ARM64 macOS GLib & Ruby | ||
| runs-on: macos-latest | ||
| runs-on: macos-26 | ||
| if: ${{ !contains(github.event.pull_request.title, 'WIP') }} |
8239181 to
df02f37
Compare
gaflight_server_listen()df02f37 to
632f010
Compare
Comment on lines
+1203
to
+1208
| auto priv = GAFLIGHT_SERVER_GET_PRIVATE(object); | ||
|
|
||
| if (priv->options) { | ||
| g_object_unref(priv->options); | ||
| priv->options = nullptr; | ||
| } |
Comment on lines
+39
to
+45
| env | sort | ||
|
|
||
| run_test_args=() | ||
| if [ -n "${RUNNER_DEBUG}" ]; then | ||
| run_test_args+=(-v) | ||
| fi | ||
| ruby test/run-test.rb "${run_test_args[@]}" |
Comment on lines
136
to
139
| macos: | ||
| name: ARM64 macOS GLib & Ruby | ||
| runs-on: macos-latest | ||
| runs-on: macos-26 | ||
| if: ${{ !contains(github.event.pull_request.title, 'WIP') }} |
eab1c4a to
e5960c4
Compare
1306697 to
1201244
Compare
Comment on lines
23
to
26
| omit("Arrow Flight is required") unless defined?(ArrowFlight) | ||
| omit("Unstable on Windows") if Gem.win_platform? | ||
| omit("Unstable on x86_64 macOS") if /x86_64-darwin/.match?(RUBY_PLATFORM) | ||
| omit("Unstable on macOS") if /darwin/.match?(RUBY_PLATFORM) | ||
| require_gi_bindings(3, 4, 7) |
Comment on lines
+18
to
+25
| module ArrowFlight | ||
| class Criteria | ||
| alias_method :initialize_raw, :initialize | ||
| def initialize(expression) | ||
| initialize_raw(expression) | ||
| @expression = expression | ||
| end | ||
| end |
Comment on lines
23
to
29
| def test_data | ||
| data = "data" | ||
| ticket = ArrowFlight::Ticket.new(data) | ||
| data_bytes = GLib::Bytes.new(data) | ||
| ticket = ArrowFlight::Ticket.new(data_bytes) | ||
| assert_equal(data, | ||
| ticket.data.to_s) | ||
| end |
1201244 to
63e8f97
Compare
63e8f97 to
f675c66
Compare
Comment on lines
23
to
26
| omit("Arrow Flight is required") unless defined?(ArrowFlight) | ||
| omit("Unstable on Windows") if Gem.win_platform? | ||
| omit("Unstable on x86_64 macOS") if /x86_64-darwin/.match?(RUBY_PLATFORM) | ||
| omit("Unstable on macOS") if /darwin/.match?(RUBY_PLATFORM) | ||
| require_gi_bindings(3, 4, 7) |
Comment on lines
20
to
23
| @server = nil | ||
| omit("Unstable on Windows") if Gem.win_platform? | ||
| omit("Unstable on macOS") if /darwin/.match?(RUBY_PLATFORM) | ||
| @server = Helper::Server.new |
f675c66 to
8670d78
Compare
Comment on lines
+32
to
+33
| alias_method :initialize_raw, :initialize | ||
| def initialize(data) |
Comment on lines
+31
to
+32
| alias_method :initialize_raw, :initialize | ||
| def initialize(expression) |
Comment on lines
24
to
27
| data = "data" | ||
| ticket = ArrowFlight::Ticket.new(data) | ||
| data_bytes = GLib::Bytes.new(data) | ||
| ticket = ArrowFlight::Ticket.new(data_bytes) | ||
| assert_equal(data, |
Comment on lines
24
to
27
| expression = "expression" | ||
| criteria = ArrowFlight::Criteria.new(expression) | ||
| expression_bytes = GLib::Bytes.new(expression) | ||
| criteria = ArrowFlight::Criteria.new(expression_bytes) | ||
| assert_equal(expression, |
Comment on lines
136
to
139
| macos: | ||
| name: ARM64 macOS GLib & Ruby | ||
| runs-on: macos-latest | ||
| runs-on: macos-26 | ||
| if: ${{ !contains(github.event.pull_request.title, 'WIP') }} |
| omit("Arrow Flight is required") unless defined?(ArrowFlight) | ||
| omit("Unstable on Windows") if Gem.win_platform? | ||
| omit("Unstable on x86_64 macOS") if /x86_64-darwin/.match?(RUBY_PLATFORM) | ||
| omit("Unstable on macOS") if /darwin/.match?(RUBY_PLATFORM) |
8670d78 to
7ff0901
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
c_glib/arrow-flight-glib/server.cpp:1249
GAFlightServerPrivate::optionsis never initialized ingaflight_server_init(). The code currently relies on instance-private memory being zeroed; if that assumption changes,dispose/get_propertymay read or unref a garbage pointer. Initialize it explicitly when constructing the instance.
static void
gaflight_server_init(GAFlightServer *object)
{
auto priv = GAFLIGHT_SERVER_GET_PRIVATE(object);
new (&(priv->server)) gaflight::Server(object);
}
… `gaflight_server_listen()` `gaflight_server_listen()` may use options' auth handler later. So we should refer the given options while the server is alive.
7ff0901 to
dba843b
Compare
| omit("Arrow Flight is required") unless defined?(ArrowFlight) | ||
| omit("Unstable on Windows") if Gem.win_platform? | ||
| omit("Unstable on x86_64 macOS") if /x86_64-darwin/.match?(RUBY_PLATFORM) | ||
| omit("Unstable on macOS") if /darwin/.match?(RUBY_PLATFORM) |
Member
Author
|
+1 |
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
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.
Rationale for this change
gaflight_server_listen()may use options' auth handler later. So we should refer the given options while the server is alive. If we don't refer the given options, we may touch freed auth handler later.The argument of
ArrowFlight::Criteria.new/ArrowFlight::Ticket.newshould be referred to protect from GC in Ruby.I could fix some problems by
GC.stress = truebut I couldn't fix some problems that are reproduced only on GitHub Actions. I omit tests for them for now.What changes are included in this PR?
gaflight_server_listen()ArrowFlight::Criteria.newArrowFlight::Ticket.newAre these changes tested?
Yes.
Are there any user-facing changes?
Yes.