(celeborn) branch branch-0.6 updated: [CELEBORN-2433] Fix Utils.tryWithResources evaluating by-name resource expression twice

[chengpan]

This is an automated email from the ASF dual-hosted git repository.

pan3793 pushed a commit to branch branch-0.6
in repository https://gitbox.apache.org/repos/asf/celeborn.git


The following commit(s) were added to refs/heads/branch-0.6 by this push:
     new d54462e48 [CELEBORN-2433] Fix Utils.tryWithResources evaluating 
by-name resource expression twice
d54462e48 is described below

commit d54462e483aa78445df3e5a5d2b5a6c76808dccf
Author: yangjie01 <yangji...@baidu.com>
AuthorDate: Thu Aug 20 22:20:19 2026 +0800

    [CELEBORN-2433] Fix Utils.tryWithResources evaluating by-name resource 
expression twice
    
    ### What changes were proposed in this pull request?
    
    `Utils.tryWithResources` declared its resource parameter by-name (`f: => 
R`) but referenced it twice: `val res = f` evaluated the caller's expression 
once, then `func(f)` evaluated it a *second* time. The instance handed to 
`func` was therefore never the one closed in `finally`. This PR passes the 
already-evaluated `res` to `func` (one line), and adds a guard test to 
`UtilsSuite` covering both halves of the helper's contract: the expression 
evaluates exactly once and the instance rece [...]
    
    ### Why are the changes needed?
    
    Every call with a non-idempotent resource expression opened the resource 
twice and leaked one handle. Current callers affected (CLI): `CliConfigManager` 
opens its config file twice per read and leaks one `FileInputStream` (which 
also blocks file deletion on Windows); `CliVersionProvider` leaks one 
`BufferedSource` per lookup. Concise repro against current main:
    
    ```scala
    val evaluations = new AtomicInteger(0)
    val received = Utils.tryWithResources {
      evaluations.incrementAndGet()
      new Closeable { var closed = false; def close() = closed = true }
    }(res => res)
    // main: evaluations == 2, received.closed == false (leaked)
    ```
    
    ### Does this PR resolve a correctness bug?
    
    - [ ] Yes
    
    ### Does this PR introduce _any_ user-facing change?
    
    - [ ] Yes
    
    No: internal utility fix; the only observable effect is that the CLI no 
longer opens files twice / leaks handles.
    
    ### How was this patch tested?
    
    - Guard test verified red/green: against the unfixed tree the assertions 
fail (`2 did not equal 1`, `received.closed == false`); they pass with the fix.
    
    Closes #3814 from LuciferYang/fix-trywithresources-double-eval.
    
    Authored-by: yangjie01 <yangji...@baidu.com>
    Signed-off-by: Cheng Pan <cheng...@apache.org>
    (cherry picked from commit ba4fa59f0279bee81e7cfa838e083a603d331ff2)
    Signed-off-by: Cheng Pan <cheng...@apache.org>
---
 .../org/apache/celeborn/common/util/Utils.scala    |  2 +-
 .../apache/celeborn/common/util/UtilsSuite.scala   | 33 ++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/common/src/main/scala/org/apache/celeborn/common/util/Utils.scala 
b/common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
index 109fdc326..55a0db173 100644
--- a/common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
+++ b/common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
@@ -1107,7 +1107,7 @@ object Utils extends Logging {
   def tryWithResources[R <: Closeable, U](f: => R)(func: R => U): U = {
     val res = f
     try {
-      func(f)
+      func(res)
     } finally {
       if (null != res) {
         res.close()
diff --git 
a/common/src/test/scala/org/apache/celeborn/common/util/UtilsSuite.scala 
b/common/src/test/scala/org/apache/celeborn/common/util/UtilsSuite.scala
index 7cadaf07e..f212428b0 100644
--- a/common/src/test/scala/org/apache/celeborn/common/util/UtilsSuite.scala
+++ b/common/src/test/scala/org/apache/celeborn/common/util/UtilsSuite.scala
@@ -17,8 +17,10 @@
 
 package org.apache.celeborn.common.util
 
+import java.io.Closeable
 import java.util
 import java.util.Collections
+import java.util.concurrent.atomic.AtomicInteger
 
 import org.scalatest.matchers.must.Matchers.contain
 import org.scalatest.matchers.should.Matchers.convertToAnyShouldWrapper
@@ -278,4 +280,35 @@ class UtilsSuite extends CelebornFunSuite {
       celebornConf)
     assert(testInstance.isInstanceOf[DefaultIdentityProvider])
   }
+
+  test("tryWithResources should evaluate the resource expression once and 
close the resource given to the function") {
+    val evaluations = new AtomicInteger(0)
+
+    class Resource extends Closeable {
+      var closed: Boolean = false
+      override def close(): Unit = closed = true
+    }
+
+    val received = Utils.tryWithResources {
+      evaluations.incrementAndGet()
+      new Resource
+    }(res => res)
+
+    assert(evaluations.get() == 1)
+    assert(received.closed)
+
+    var thrownReceived: Resource = null
+    intercept[RuntimeException] {
+      Utils.tryWithResources {
+        evaluations.incrementAndGet()
+        new Resource
+      } { res =>
+        thrownReceived = res
+        throw new RuntimeException("func failed")
+      }
+    }
+    assert(evaluations.get() == 2)
+    assert(thrownReceived.closed)
+    assert(thrownReceived ne received)
+  }
 }