Copilot commented on code in PR #4917: URL: https://github.com/apache/arrow-adbc/pull/4917#discussion_r4307489756
########## csharp/src/Apache.Arrow.Adbc/C/IdleCollector.cs: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Apache.Arrow.Adbc.C +{ + /// <summary> + /// Tracks the databases an exported driver has open and runs a collection once + /// the last one has been released and none has been opened for <see cref="Delay"/>. + /// </summary> + internal sealed class IdleCollector + { + private readonly Action _collect; + private TimeSpan? _delay; + private int _open; + private int _generation; + private int _collectedGeneration = -1; + + public IdleCollector(Action collect, TimeSpan? delay) + { + _collect = collect; + Delay = delay; + } + + /// <summary> + /// How long to wait for the driver to stay idle before collecting, or <c>null</c> + /// to never collect. + /// </summary> + public TimeSpan? Delay + { + get => _delay; + set + { + if (value < TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException(nameof(value), "The delay cannot be negative."); + } + _delay = value; Review Comment: The setter accepts delays outside `Task.Delay(TimeSpan)`'s supported range, such as `TimeSpan.MaxValue`. Scheduling then throws during database or driver release, after the exporter has disposed the object, freed its GCHandle, and cleared `private_data`; a completed release is therefore reported as a failure. Reject unsupported delays here. A portable cap of `int.MaxValue` milliseconds avoids runtime-dependent timer limits; add boundary tests for the constructor and setter. ########## csharp/src/Apache.Arrow.Adbc/C/IdleCollector.cs: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Apache.Arrow.Adbc.C +{ + /// <summary> + /// Tracks the databases an exported driver has open and runs a collection once + /// the last one has been released and none has been opened for <see cref="Delay"/>. + /// </summary> + internal sealed class IdleCollector + { + private readonly Action _collect; + private TimeSpan? _delay; + private int _open; + private int _generation; + private int _collectedGeneration = -1; + + public IdleCollector(Action collect, TimeSpan? delay) + { + _collect = collect; + Delay = delay; + } + + /// <summary> + /// How long to wait for the driver to stay idle before collecting, or <c>null</c> + /// to never collect. + /// </summary> + public TimeSpan? Delay + { + get => _delay; + set + { + if (value < TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException(nameof(value), "The delay cannot be negative."); + } + _delay = value; + } + } + + public void DatabaseOpened() + { + Interlocked.Increment(ref _open); + Interlocked.Increment(ref _generation); + } + + public void DatabaseReleased() + { + if (Interlocked.Decrement(ref _open) == 0) + { + Schedule(); + } + } + + public void DriverReleased() + { + if (Volatile.Read(ref _open) == 0) + { + Schedule(); + } + } + + private void Schedule() + { + TimeSpan? delay = _delay; + if (delay == null) + { + return; + } + + int generation = Volatile.Read(ref _generation); + Task.Delay(delay.Value).ContinueWith(_ => CollectIfIdle(generation), TaskScheduler.Default); + } + + private void CollectIfIdle(int generation) + { + // Skip if a database was opened since this was scheduled; its release + // schedules a collection of its own. + if (Volatile.Read(ref _open) != 0 || Volatile.Read(ref _generation) != generation) + { + return; + } + + // Releasing the last database and then the driver schedules twice for the + // same idle period, but one collection is enough. + if (Interlocked.Exchange(ref _collectedGeneration, generation) == generation) Review Comment: If the last-database collection finishes before `DriverReleased()`, the driver-release request uses the already-collected generation and is skipped here. The earlier GC ran while the driver was still rooted by its GCHandle, so objects made unreachable by releasing the driver get no idle collection. Subsequent driver-only lifetimes are also skipped until a database opens. Allow driver release to request a fresh collection after completed work, while still coalescing pending requests. Cover both delayed driver release and repeated driver-only lifetimes in tests. ########## csharp/src/Apache.Arrow.Adbc/C/IdleCollector.cs: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Apache.Arrow.Adbc.C +{ + /// <summary> + /// Tracks the databases an exported driver has open and runs a collection once + /// the last one has been released and none has been opened for <see cref="Delay"/>. + /// </summary> + internal sealed class IdleCollector + { + private readonly Action _collect; + private TimeSpan? _delay; + private int _open; + private int _generation; + private int _collectedGeneration = -1; + + public IdleCollector(Action collect, TimeSpan? delay) + { + _collect = collect; + Delay = delay; + } + + /// <summary> + /// How long to wait for the driver to stay idle before collecting, or <c>null</c> + /// to never collect. + /// </summary> + public TimeSpan? Delay + { + get => _delay; + set + { + if (value < TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException(nameof(value), "The delay cannot be negative."); + } + _delay = value; + } + } + + public void DatabaseOpened() + { + Interlocked.Increment(ref _open); + Interlocked.Increment(ref _generation); + } + + public void DatabaseReleased() + { + if (Interlocked.Decrement(ref _open) == 0) + { + Schedule(); + } + } + + public void DriverReleased() + { + if (Volatile.Read(ref _open) == 0) + { + Schedule(); + } + } + + private void Schedule() + { + TimeSpan? delay = _delay; + if (delay == null) + { + return; + } + + int generation = Volatile.Read(ref _generation); + Task.Delay(delay.Value).ContinueWith(_ => CollectIfIdle(generation), TaskScheduler.Default); + } + + private void CollectIfIdle(int generation) + { + // Skip if a database was opened since this was scheduled; its release + // schedules a collection of its own. + if (Volatile.Read(ref _open) != 0 || Volatile.Read(ref _generation) != generation) Review Comment: Setting `IdleCollectionDelay` to `null` after a collection is scheduled does not disable that pending collection: this callback never checks the setting again, so it still forces a full GC. Recheck whether collection is enabled before admitting the callback, with synchronized access to the mutable delay, or cancel pending work when disabling. Add a test that disables collection after `DatabaseReleased()` but before the delay expires. ########## csharp/src/Apache.Arrow.Adbc/C/IdleCollector.cs: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Apache.Arrow.Adbc.C +{ + /// <summary> + /// Tracks the databases an exported driver has open and runs a collection once + /// the last one has been released and none has been opened for <see cref="Delay"/>. + /// </summary> + internal sealed class IdleCollector + { + private readonly Action _collect; + private TimeSpan? _delay; + private int _open; + private int _generation; + private int _collectedGeneration = -1; + + public IdleCollector(Action collect, TimeSpan? delay) + { + _collect = collect; + Delay = delay; + } + + /// <summary> + /// How long to wait for the driver to stay idle before collecting, or <c>null</c> + /// to never collect. + /// </summary> + public TimeSpan? Delay + { + get => _delay; + set + { + if (value < TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException(nameof(value), "The delay cannot be negative."); + } + _delay = value; + } + } + + public void DatabaseOpened() + { + Interlocked.Increment(ref _open); + Interlocked.Increment(ref _generation); + } + + public void DatabaseReleased() + { + if (Interlocked.Decrement(ref _open) == 0) + { + Schedule(); + } + } + + public void DriverReleased() + { + if (Volatile.Read(ref _open) == 0) + { + Schedule(); + } + } + + private void Schedule() + { + TimeSpan? delay = _delay; + if (delay == null) + { + return; + } + + int generation = Volatile.Read(ref _generation); + Task.Delay(delay.Value).ContinueWith(_ => CollectIfIdle(generation), TaskScheduler.Default); + } + + private void CollectIfIdle(int generation) + { + // Skip if a database was opened since this was scheduled; its release + // schedules a collection of its own. + if (Volatile.Read(ref _open) != 0 || Volatile.Read(ref _generation) != generation) Review Comment: The idle check races with `DatabaseOpened()`. A callback can read `_open == 0`, then an opening thread increments `_open` and pauses before incrementing `_generation`. The callback still sees the old generation and proceeds to force GC despite the reopen, introducing full-collection pauses during active work. Coordinate database opening and collection admission under one synchronization protocol, and add a regression test that controls this interleaving. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
