Create SafeBoosterHandle and SafeDataSetHandle

[sharwell]

These safe handles were created based on the crash data in https://dev.azure.com/dnceng/public/_build/results?buildId=449745&view=logs.

[codemzs]

[codecov]

Codecov Report

Merging #4539 into master will increase coverage by 0.02%.
The diff coverage is 88%.

@@            Coverage Diff             @@
##           master    #4539      +/-   ##
==========================================
+ Coverage   75.12%   75.14%   +0.02%     
==========================================
  Files         908      908              
  Lines      160214   160223       +9     
  Branches    17250    17250              
==========================================
+ Hits       120355   120395      +40     
+ Misses      35045    35009      -36     
- Partials     4814     4819       +5

Flag	Coverage Δ
#Debug	75.14% <88%> (+0.02%)	⬆️
#production	70.54% <88%> (+0.02%)	⬆️
#test	90.31% <ø> (-0.01%)	⬇️

Impacted Files	Coverage Δ
.../Microsoft.ML.LightGbm/WrappedLightGbmInterface.cs	87.71% <100%> (+3.99%)	⬆️
...rc/Microsoft.ML.LightGbm/WrappedLightGbmDataset.cs	74.21% <71.42%> (-2.12%)	⬇️
...rc/Microsoft.ML.LightGbm/WrappedLightGbmBooster.cs	83.8% <75%> (-0.63%)	⬇️
...rosoft.ML.AutoML/ColumnInference/TextFileSample.cs	59.6% <0%> (-2.65%)	⬇️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs	76.08% <0%> (-0.4%)	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.26% <0%> (+0.15%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	85.31% <0%> (+0.2%)	⬆️
src/Microsoft.ML.AutoML/Sweepers/Parameters.cs	85.16% <0%> (+0.84%)	⬆️
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs	86.55% <0%> (+3.36%)	⬆️
src/Microsoft.ML.Core/Data/ProgressReporter.cs	77.94% <0%> (+6.98%)	⬆️
... and 2 more

[sharwell]

📝 The change to SafeHandle means that the finalizer for SafeBoosterHandle will now be invoked at some point if the call to BoosterCreate succeeds but an exception is thrown later in this constructor (i.e. an exception prevents the caller from being able to use Booster.Dispose()). Ideally, code which constructs disposable objects should be robust in ensuring created objects are disposed if a failure would prevent those objects from being disposed by the caller. I'm not ready to submit the change to ensure all the callers dispose of objects deterministically on exceptional paths, but I'd like to merge this one in the meantime.

[eerhardt]

Doing a quick scan of the usages of the Booster and Dataset classes, they are always disposed of in the product code (one in a using and the other in a try-finally. The only case I saw where they aren't disposed of is in the tests.

machinelearning/test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs

Line 494 in 8221dac

var gbmNative = WrappedLightGbmTraining.Train(ch, pch, gbmParams, gbmDataSet, numIteration: numberOfTrainingIterations);

machinelearning/test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs

Line 475 in 8221dac

var gbmDataSet = new Trainers.LightGbm.Dataset(sampleValueGroupedByColumn, sampleIndicesGroupedByColumn, _columnNumber, sampleNonZeroCntPerColumn, _rowNumber, _rowNumber, "", floatLabels);

[sharwell]

The Booster class is disposed if the constructor returns without throwing an exception. However, if LightGbmInterfaceUtils.Check throws an exception from the constructor itself, the SafeBoosterHandle will have been created by the call to BoosterCreate but never stored in an object that is later disposed. Prior to the use of a SafeHandle, an instance in this scenario would never be cleaned up. Now it will be cleaned up by the finalizer.