Set CA1001 to "Error" and resolved issues (#1826)

* Set CA1001 to "Error". * Set up BrowserFetcher as IDisposable and disposed of disposable resources. * Set up TaskQueue as IDisposable. * Converted WaitTask to IDisposable. * Disposed of newly disposable classes. * Updated usages, samples, and demos of BrowserFetcher so that it contains a using statement. There are some cases where the example code is pulling a previous version of the NuGet package, which means BrowserFetcher won't be disposable for that project. * Removed unnecessary disposal of _timeoutTimer. * Added Transport event unsubscription to the Dispose() method of Connection.cs to resolve issues of transport events firing against the _callbackQueue's disposed semaphore. Added ScreenshotTaskQueue disposal to Browser.cs after the async disposals occur. * Updated TaskQueue's disposal logic to ensure it waits for all existing queued calls to finish before it disposes of the semaphore. Updated TaskQueue to implement IAsyncDisposable and delegated the Dispose method to use DisposeAsync, in a discard task. Set Browser to call TaskQueue.DisposeAsync() rather than .Dispose(). Set Page to call TaskQueue.DisposeAsync() instead of .Dispose(). Also delegated Dispose's discard task to DisposeAsync(), as they were logically equivalent.
hardkoded · Oct 1, 2021 · 47798cc · 47798cc
1 parent c15c5c4
commit 47798cc
Show file tree

Hide file tree

Showing 24 changed files with 169 additions and 48 deletions.
diff --git a/demos/PuppeteerSharpPdfDemo/Program.cs b/demos/PuppeteerSharpPdfDemo/Program.cs
@@ -16,7 +16,8 @@ public static async Task Main(string[] args)
             };
 
             Console.WriteLine("Downloading chromium");
-            await new BrowserFetcher().DownloadAsync();
+            using var browserFetcher = new BrowserFetcher();
+            await browserFetcher.DownloadAsync();
 
             Console.WriteLine("Navigating google");
             using (var browser = await Puppeteer.LaunchAsync(options))

diff --git a/demos/PuppeteerSharpPdfDemo/PuppeteerSharpPdfDemo-Local.csproj b/demos/PuppeteerSharpPdfDemo/PuppeteerSharpPdfDemo-Local.csproj
@@ -3,7 +3,7 @@
   <PropertyGroup>
     <OutputType>Exe</OutputType>
     <TargetFrameworks>netcoreapp3.1;net471</TargetFrameworks>
-    <LangVersion>7.3</LangVersion>
+    <LangVersion>8.0</LangVersion>
   </PropertyGroup>
   <PropertyGroup Condition="'$(TargetFramework)'=='net471'">
     <RuntimeIdentifier>win7-x64</RuntimeIdentifier>

diff --git a/docfx_project/api/index.md b/docfx_project/api/index.md
@@ -7,7 +7,8 @@ Puppeteer Sharp is a .NET port of the official [Node.JS Puppeteer API](https://g
 ## Take screenshots
 
 ```cs
-await new BrowserFetcher().DownloadAsync(BrowserFetcher.DefaultRevision);
+using var browserFetcher = new BrowserFetcher();
+await browserFetcher.DownloadAsync(BrowserFetcher.DefaultRevision);
 var browser = await Puppeteer.LaunchAsync(new LaunchOptions
 {
     Headless = true
@@ -45,7 +46,8 @@ await page.PdfAsync(outputFile);
 ### Generate PDF files with custom options
 
 ```cs
-await new BrowserFetcher().DownloadAsync(BrowserFetcher.DefaultRevision);
+using var browserFetcher = new BrowserFetcher();
+await browserFetcher.DownloadAsync(BrowserFetcher.DefaultRevision);
 var browser = await Puppeteer.LaunchAsync(new LaunchOptions
 {
     Headless = true

diff --git a/docfx_project/examples/ChromeExtension.md b/docfx_project/examples/ChromeExtension.md
@@ -10,10 +10,11 @@ You need to test a chrome extension
 Use `Puppeteer.LaunchAsync` passing arguments specifying to load your extension.
 
 ```cs
-await new BrowserFetcher().DownloadAsync(BrowserFetcher.DefaultRevision);
+using var browserFetcher = new BrowserFetcher();
+await browserFetcher.DownloadAsync(BrowserFetcher.DefaultRevision);
 
 var pathToExtension = "path/to/extension";
-var launhcOptions = new LaunhcOptions()
+var launchOptions = new LaunchOptions()
 {
     Headless = false,
     Args = new []

diff --git a/docfx_project/examples/Page.ScreenshotAsync.md b/docfx_project/examples/Page.ScreenshotAsync.md
@@ -10,7 +10,8 @@ You need to take an screenshot of a page.
 Use `Page.ScreenshotAsync` passing a file path as an argument.
 
 ```cs
-new BrowserFetcher().DownloadAsync(BrowserFetcher.DefaultRevision);
+using var browserFetcher = new BrowserFetcher();
+await browserFetcher.DownloadAsync(BrowserFetcher.DefaultRevision);
 
 var url = "https://www.somepage.com";
 var file = ".\\somepage.jpg";

diff --git a/docfx_project/examples/ReuseChrome.md b/docfx_project/examples/ReuseChrome.md
@@ -13,7 +13,7 @@ Use `BrowserFetcherOptions` to specify the full path for where to download Chrom
 
 ```
 var browserFetcherOptions = new BrowserFetcherOptions { Path = downloadPath };
-var browserFetcher = new BrowserFetcher(browserFetcherOptions);
+using var browserFetcher = new BrowserFetcher(browserFetcherOptions);
 await browserFetcher.DownloadAsync(BrowserFetcher.DefaultRevision);
 ```
 

diff --git a/docfx_project/examples/index.md b/docfx_project/examples/index.md
@@ -7,7 +7,8 @@ Puppeteer Sharp is a .NET port of the official [Node.JS Puppeteer API](https://g
 ## Take screenshots
 
 ```cs
-await new BrowserFetcher().DownloadAsync(BrowserFetcher.DefaultChromiumRevision);
+using var browserFetcher = new BrowserFetcher();
+await browserFetcher.DownloadAsync(BrowserFetcher.DefaultChromiumRevision);
 var browser = await Puppeteer.LaunchAsync(new LaunchOptions
 {
     Headless = true
@@ -30,7 +31,8 @@ await page.SetViewportAsync(new ViewPortOptions
 ## Generate PDF files
 
 ```cs
-await new BrowserFetcher().DownloadAsync(BrowserFetcher.DefaultChromiumRevision);
+using var browserFetcher = new BrowserFetcher();
+await browserFetcher.DownloadAsync(BrowserFetcher.DefaultChromiumRevision);
 var browser = await Puppeteer.LaunchAsync(new LaunchOptions
 {
     Headless = true

diff --git a/lib/PuppeteerSharp.DevicesFetcher/Program.cs b/lib/PuppeteerSharp.DevicesFetcher/Program.cs
@@ -27,7 +27,7 @@ static async Task Main(string[] args)
             }
 
             string chromeVersion = null;
-            var fetcher = new BrowserFetcher();
+            using var fetcher = new BrowserFetcher();
 
             await fetcher.DownloadAsync();
             await using (var browser = await Puppeteer.LaunchAsync(new LaunchOptions()))

diff --git a/lib/PuppeteerSharp.Tests/FixturesTests/FixturesTests.cs b/lib/PuppeteerSharp.Tests/FixturesTests/FixturesTests.cs
@@ -19,9 +19,10 @@ public FixturesTests(ITestOutputHelper output) : base(output) { }
         public async Task ShouldDumpBrowserProcessStderr()
         {
             var success = false;
+            using var browserFetcher = new BrowserFetcher(Product.Chrome);
             var process = GetTestAppProcess(
                 "PuppeteerSharp.Tests.DumpIO",
-                $"\"{(await new BrowserFetcher(Product.Chrome).GetRevisionInfoAsync()).ExecutablePath}\"");
+                $"\"{(await browserFetcher.GetRevisionInfoAsync().ConfigureAwait(false)).ExecutablePath}\"");
 
             process.ErrorDataReceived += (_, e) =>
             {
@@ -38,8 +39,9 @@ public async Task ShouldDumpBrowserProcessStderr()
         public async Task ShouldCloseTheBrowserWhenTheConnectedProcessCloses()
         {
             var browserClosedTaskWrapper = new TaskCompletionSource<bool>();
+            using var browserFetcher = new BrowserFetcher(Product.Chrome);
             var ChromiumLauncher = new ChromiumLauncher(
-                (await new BrowserFetcher(Product.Chrome).GetRevisionInfoAsync()).ExecutablePath,
+                (await browserFetcher.GetRevisionInfoAsync()).ExecutablePath,
                 new LaunchOptions { Headless = true });
 
             await ChromiumLauncher.StartAsync().ConfigureAwait(false);

diff --git a/lib/PuppeteerSharp.Tests/LauncherTests/BrowserFetcherTests.cs b/lib/PuppeteerSharp.Tests/LauncherTests/BrowserFetcherTests.cs
@@ -27,7 +27,7 @@ public BrowserFetcherTests(ITestOutputHelper output) : base(output)
         [PuppeteerFact]
         public async Task ShouldDownloadAndExtractLinuxBinary()
         {
-            var browserFetcher = Puppeteer.CreateBrowserFetcher(new BrowserFetcherOptions
+            using var browserFetcher = Puppeteer.CreateBrowserFetcher(new BrowserFetcherOptions
             {
                 Platform = Platform.Linux,
                 Path = _downloadsFolder,
@@ -77,7 +77,7 @@ public async Task ShouldDownloadAndExtractLinuxBinary()
         [PuppeteerFact]
         public async Task ShouldDownloadAndExtractFirefoxLinuxBinary()
         {
-            var browserFetcher = Puppeteer.CreateBrowserFetcher(new BrowserFetcherOptions
+            using var browserFetcher = Puppeteer.CreateBrowserFetcher(new BrowserFetcherOptions
             {
                 Platform = Platform.Linux,
                 Path = _downloadsFolder,
@@ -165,4 +165,4 @@ private void EnsureDownloadsFolderIsDeleted()
             }
         }
     }
-}
+}
diff --git a/lib/PuppeteerSharp.Tests/PageTests/PdfTests.cs b/lib/PuppeteerSharp.Tests/PageTests/PdfTests.cs
@@ -28,7 +28,7 @@ public async Task Usage()
 
             #region PdfAsync
 
-            var browserFetcher = new BrowserFetcher();
+            using var browserFetcher = new BrowserFetcher();
             await browserFetcher.DownloadAsync();
             await using var browser = await Puppeteer.LaunchAsync(new LaunchOptions {Headless = true});
             await using var page = await browser.NewPageAsync();

diff --git a/lib/PuppeteerSharp.Tests/PuppeteerLoaderFixture.cs b/lib/PuppeteerSharp.Tests/PuppeteerLoaderFixture.cs
@@ -21,7 +21,8 @@ public void Dispose()
 
         private async Task SetupAsync()
         {
-            var downloaderTask = new BrowserFetcher(TestConstants.IsChrome ? Product.Chrome : Product.Firefox).DownloadAsync();
+            using var browserFetcher = new BrowserFetcher(TestConstants.IsChrome ? Product.Chrome : Product.Firefox);
+            var downloaderTask = browserFetcher.DownloadAsync();
 
             Server = SimpleServer.Create(TestConstants.Port, TestUtils.FindParentDirectory("PuppeteerSharp.TestServer"));
             HttpsServer = SimpleServer.CreateHttps(TestConstants.HttpsPort, TestUtils.FindParentDirectory("PuppeteerSharp.TestServer"));
@@ -32,4 +33,4 @@ private async Task SetupAsync()
             await Task.WhenAll(downloaderTask, serverStart, httpsServerStart);
         }
     }
-}
+}
diff --git a/lib/PuppeteerSharp.Tests/ScreenshotTests/PageScreenshotTests.cs b/lib/PuppeteerSharp.Tests/ScreenshotTests/PageScreenshotTests.cs
@@ -61,7 +61,7 @@ public async Task Usage()
                 fileInfo.Delete();
             }
             #region ScreenshotAsync
-            var browserFetcher = new BrowserFetcher();
+            using var browserFetcher = new BrowserFetcher();
             await browserFetcher.DownloadAsync();
             await using var browser = await Puppeteer.LaunchAsync(
                 new LaunchOptions { Headless = true });

diff --git a/lib/PuppeteerSharp.ruleset b/lib/PuppeteerSharp.ruleset
@@ -68,7 +68,7 @@
     <Rule Id="CA1062" Action="Error" /> <!-- Error CA1031: Modify 'DeleteAsync' to catch a more specific allowed exception type, or rethrow the exception. (CA1031)-->
     <Rule Id="CA1063" Action="Error" /> <!-- Error CA1063: Provide an overridable implementation of Dispose(bool) -->
     <Rule Id="CA1064" Action="Error" /> <!-- Error CA1064: Exceptions should be public (CA1064)-->
-    <Rule Id="CA1001" Action="None" /> <!-- Error CA1001: A class owns a disposable -->
+    <Rule Id="CA1001" Action="Error" /> <!-- Error CA1001: A class owns a disposable -->
     <Rule Id="CA1304" Action="Error" /> <!-- Error CA1304: The behavior of 'string.ToLower()' could vary based on the current user's locale settings. Replace this call in 'JSHandle.ToString()' with a call to 'string.ToLower(CultureInfo)'. (CA1304) -->
     <Rule Id="CA1305" Action="Error" /> <!-- String.Format with culture-->
     <Rule Id="CA1720" Action="Error" /> <!-- Error SA1720: Name contains type-->

diff --git a/lib/PuppeteerSharp/Browser.cs b/lib/PuppeteerSharp/Browser.cs
@@ -547,7 +547,10 @@ public void Dispose()
         /// created by Puppeteer.
         /// </summary>
         /// <param name="disposing">Indicates whether disposal was initiated by <see cref="Dispose()"/> operation.</param>
-        protected virtual void Dispose(bool disposing) => _ = CloseAsync();
+        protected virtual void Dispose(bool disposing) => _ = CloseAsync()
+            .ContinueWith(
+                _ => ScreenshotTaskQueue.DisposeAsync(),
+                TaskScheduler.Default);
 
         #endregion
 

diff --git a/lib/PuppeteerSharp/BrowserFetcher.cs b/lib/PuppeteerSharp/BrowserFetcher.cs
@@ -30,7 +30,7 @@ namespace PuppeteerSharp
     /// var browser = await await Puppeteer.LaunchAsync(new LaunchOptions { ExecutablePath = revisionInfo.ExecutablePath});
     /// </code>
     /// </example>
-    public class BrowserFetcher
+    public class BrowserFetcher : IDisposable
     {
         private static readonly Dictionary<Product, string> _hosts = new Dictionary<Product, string>
         {
@@ -51,6 +51,7 @@ public class BrowserFetcher
         };
 
         private readonly WebClient _webClient = new WebClient();
+        private bool _isDisposed;
 
         /// <summary>
         /// Default Chromium revision.
@@ -626,5 +627,33 @@ private static string GetArchiveName(Product product, Platform platform, string
 
         private static string GetDownloadURL(Product product, Platform platform, string host, string revision)
             => string.Format(CultureInfo.CurrentCulture, _downloadUrls[(product, platform)], host, revision, GetArchiveName(product, platform, revision));
+
+        /// <summary>
+        /// Disposes of any disposable members in <see cref="BrowserFetcher" />.
+        /// </summary>
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+
+        /// <summary>
+        /// Disposes of any disposable members in <see cref="BrowserFetcher" />.
+        /// </summary>
+        /// <param name="disposing"></param>
+        protected virtual void Dispose(bool disposing)
+        {
+            if (_isDisposed)
+            {
+                return;
+            }
+
+            if (disposing)
+            {
+                _webClient.Dispose();
+            }
+
+            _isDisposed = true;
+        }
     }
 }
diff --git a/lib/PuppeteerSharp/Connection.cs b/lib/PuppeteerSharp/Connection.cs
@@ -330,7 +330,10 @@ public void Dispose()
         protected virtual void Dispose(bool disposing)
         {
             Close("Connection disposed");
+            Transport.MessageReceived -= Transport_MessageReceived;
+            Transport.Closed -= Transport_Closed;
             Transport.Dispose();
+            _callbackQueue.Dispose();
         }
         #endregion
     }

diff --git a/lib/PuppeteerSharp/DOMWorld.cs b/lib/PuppeteerSharp/DOMWorld.cs
@@ -352,26 +352,38 @@ internal Task<ElementHandle> WaitForSelectorAsync(string selector, WaitForSelect
         internal Task<ElementHandle> WaitForXPathAsync(string xpath, WaitForSelectorOptions options = null)
             => WaitForSelectorOrXPathAsync(xpath, true, options);
 
-        internal Task<JSHandle> WaitForFunctionAsync(string script, WaitForFunctionOptions options, params object[] args)
-            => new WaitTask(
-                this,
-                script,
-                false,
-                "function",
-                options.Polling,
-                options.PollingInterval,
-                options.Timeout ?? _timeoutSettings.Timeout,
-                args).Task;
+        internal async Task<JSHandle> WaitForFunctionAsync(string script, WaitForFunctionOptions options, params object[] args)
+        {
+            using var waitTask = new WaitTask(
+                 this,
+                 script,
+                 false,
+                 "function",
+                 options.Polling,
+                 options.PollingInterval,
+                 options.Timeout ?? _timeoutSettings.Timeout,
+                 args);
+
+            return await waitTask
+                .Task
+                .ConfigureAwait(false);
+        }
 
-        internal Task<JSHandle> WaitForExpressionAsync(string script, WaitForFunctionOptions options)
-            => new WaitTask(
+        internal async Task<JSHandle> WaitForExpressionAsync(string script, WaitForFunctionOptions options)
+        {
+            using var waitTask = new WaitTask(
                 this,
                 script,
                 true,
                 "function",
                 options.Polling,
                 options.PollingInterval,
-                options.Timeout ?? _timeoutSettings.Timeout).Task;
+                options.Timeout ?? _timeoutSettings.Timeout);
+
+            return await waitTask
+                .Task
+                .ConfigureAwait(false);
+        }
 
         internal Task<string> GetTitleAsync() => EvaluateExpressionAsync<string>("document.title");
 
@@ -414,15 +426,18 @@ function hasVisibleBoundingBox() {
                 }
               }";
             var polling = options.Visible || options.Hidden ? WaitForFunctionPollingOption.Raf : WaitForFunctionPollingOption.Mutation;
-            var handle = await new WaitTask(
+
+            using var waitTask = new WaitTask(
                 this,
                 predicate,
                 false,
                 $"{(isXPath ? "XPath" : "selector")} '{selectorOrXPath}'{(options.Hidden ? " to be hidden" : string.Empty)}",
                 polling,
                 null,
                 timeout,
-                new object[] { selectorOrXPath, isXPath, options.Visible, options.Hidden }).Task.ConfigureAwait(false);
+                new object[] { selectorOrXPath, isXPath, options.Visible, options.Hidden });
+
+            var handle = await waitTask.Task.ConfigureAwait(false);
 
             if (!(handle is ElementHandle elementHandle))
             {