Re: [PR] GH-50302: [GLib][Ruby][FlightRPC] Add missing references for GC [arrow]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #50401:
URL: https://github.com/apache/arrow/pull/50401#discussion_r3540404535


##########
ruby/red-arrow-flight/lib/arrow-flight/criteria.rb:
##########
@@ -0,0 +1,26 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+module ArrowFlight
+  class Criteria
+    alias_method :initialize_raw, :initialize
+    def initialize(expression)
+      initialize_raw(expression)
+      @expression = expression
+    end
+  end

Review Comment:
   This new Ruby override changes the behavior/lifetime semantics of ArrowFlight::Criteria#initialize, but there is no corresponding test under ruby/red-arrow-flight/test to ensure Criteria.new continues to work (and to protect against regressions in the GC/lifetime issue this PR targets). Adding a focused unit test (similar to ruby/red-arrow-flight/test/test-ticket.rb) would make the change safer.



##########
c_glib/test/flight/test-ticket.rb:
##########
@@ -22,23 +22,28 @@ def setup
 
   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

Review Comment:
   This test now uses GLib::Bytes to avoid Ruby GC/lifetime issues, but other c_glib Flight tests in the suite still call ArrowFlight::Ticket.new with String literals (e.g., c_glib/test/flight/test-endpoint.rb and c_glib/test/flight-sql/test-client.rb). If String arguments are unsafe without an extra reference (as described in the PR rationale), those remaining call sites may still be vulnerable to intermittent failures. Consider updating the remaining tests to pass GLib::Bytes (or otherwise keep a Ruby reference alive) for consistency.



##########
c_glib/test/flight/test-client.rb:
##########
@@ -22,7 +22,7 @@ def setup
     @server = nil
     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)

Review Comment:
   The PR rationale is to fix a macOS (arm64) crash by fixing object lifetimes, but this change omits the entire Flight client test suite on *all* macOS platforms. This prevents the macOS job from validating the fix and reduces coverage going forward. Consider narrowing the omit condition to the specific failing configuration, or (preferably) removing the omit once the underlying crash is addressed so the macOS CI run actually exercises Flight client/server behavior.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org