ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/aspnetcore
File: src/Hosting/WindowsServices/src/WebHostService.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.ComponentModel;
using System.ServiceProcess;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

namespace Microsoft.AspNetCore.Hosting.WindowsServices;

/// <summary>
///     Provides an implementation of a Windows service that hosts ASP.NET Core.
/// </summary>
[DesignerCategory("Code")]
[Obsolete("Use UseWindowsService and AddHostedService instead.")]
public class WebHostService : ServiceBase
{
    private readonly IWebHost _host;
    private bool _stopRequestedByWindows;

    /// <summary>
    /// Creates an instance of <c>WebHostService</c> which hosts the specified web application.
    /// </summary>
    /// <param name="host">The configured web host containing the web application to host in the Windows service.</param>
    public WebHostService(IWebHost host)
    {
        _host = host ?? throw new ArgumentNullException(nameof(host));
    }

    /// <summary>
    /// This method is not intended for direct use. Its sole purpose is to allow
    /// the service to be started by the tests.
    /// </summary>
    internal void Start() => OnStart(Array.Empty<string>());

    /// <inheritdoc />
    protected sealed override void OnStart(string[] args)
    {
        OnStarting(args);


Review comment (suggestion):
```suggestion
[Obsolete("Use UseWindowsService and AddHostedService instead.", DiagnosticId = "ASPDEPR004")]
```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.ComponentModel;
using System.ServiceProcess;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

namespace Microsoft.AspNetCore.Hosting.WindowsServices;

/// <summary>
///     Provides an implementation of a Windows service that hosts ASP.NET Core.
/// </summary>
[DesignerCategory("Code")]
[Obsolete("Use UseWindowsService and AddHostedService instead. For more information, visit https://aka.ms/aspnet/deprecate/009.", DiagnosticId = "ASPDEPR009")]
public class WebHostService : ServiceBase
{
    private readonly IWebHost _host;
    private bool _stopRequestedByWindows;

    /// <summary>
    /// Creates an instance of <c>WebHostService</c> which hosts the specified web application.
    /// </summary>
    /// <param name="host">The configured web host containing the web application to host in the Windows service.</param>
    public WebHostService(IWebHost host)
    {
        _host = host ?? throw new ArgumentNullException(nameof(host));
    }

    /// <summary>
    /// This method is not intended for direct use. Its sole purpose is to allow
    /// the service to be started by the tests.
    /// </summary>
    internal void Start() => OnStart(Array.Empty<string>());

    /// <inheritdoc />
    protected sealed override void OnStart(string[] args)
    {
        OnStarting(args);
Repository: dotnet/aspnetcore
File: src/Hosting/WindowsServices/src/WebHostWindowsServiceExtensions.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.ServiceProcess;

namespace Microsoft.AspNetCore.Hosting.WindowsServices;

/// <summary>
///     Extensions to <see cref="IWebHost"/> for hosting inside a Windows service.
/// </summary>
[Obsolete("Use UseWindowsService and AddHostedService instead.")]
public static class WebHostWindowsServiceExtensions
{
    /// <summary>
    ///     Runs the specified web application inside a Windows service and blocks until the service is stopped.
    /// </summary>
    /// <param name="host">An instance of the <see cref="IWebHost"/> to host in the Windows service.</param>
    /// <example>
    ///     This example shows how to use <see cref="RunAsService"/>.
    ///     <code>
    ///         public class Program
    ///         {
    ///             public static void Main(string[] args)
    ///             {
    ///                 var config = WebHostConfiguration.GetDefault(args);
    ///
    ///                 var host = new WebHostBuilder()
    ///                     .UseConfiguration(config)
    ///                     .Build();
    ///
    ///                 // This call will block until the service is stopped.
    ///                 host.RunAsService();
    ///             }
    ///         }
    ///     </code>
    /// </example>

Review comment (suggestion):
```suggestion
[Obsolete("Use UseWindowsService and AddHostedService instead.", DiagnosticId = "ASPDEPR004")]
```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.ServiceProcess;

namespace Microsoft.AspNetCore.Hosting.WindowsServices;

/// <summary>
///     Extensions to <see cref="IWebHost"/> for hosting inside a Windows service.
/// </summary>
[Obsolete("Use UseWindowsService and AddHostedService instead. For more information, visit https://aka.ms/aspnet/deprecate/009.", DiagnosticId = "ASPDEPR009")]
public static class WebHostWindowsServiceExtensions
{
    /// <summary>
    ///     Runs the specified web application inside a Windows service and blocks until the service is stopped.
    /// </summary>
    /// <param name="host">An instance of the <see cref="IWebHost"/> to host in the Windows service.</param>
    /// <example>
    ///     This example shows how to use <see cref="RunAsService"/>.
    ///     <code>
    ///         public class Program
    ///         {
    ///             public static void Main(string[] args)
    ///             {
    ///                 var config = WebHostConfiguration.GetDefault(args);
    ///
    ///                 var host = new WebHostBuilder()
    ///                     .UseConfiguration(config)
    ///                     .Build();
    ///
    ///                 // This call will block until the service is stopped.
    ///                 host.RunAsService();
    ///             }
    ///         }
    ///     </code>
    /// </example>
Repository: dotnet/aspnetcore
File: src/Hosting/Abstractions/src/IWebHost.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http.Features;

namespace Microsoft.AspNetCore.Hosting;

/// <summary>
/// Represents a configured web host.
/// </summary>
[Obsolete("IWebHost is obsolete. Use IHost instead.", DiagnosticId = "ASPDEPR008")]
public interface IWebHost : IDisposable
{
    /// <summary>
    /// The <see cref="IFeatureCollection"/> exposed by the configured server.
    /// </summary>
    IFeatureCollection ServerFeatures { get; }

    /// <summary>
    /// The <see cref="IServiceProvider"/> for the host.
    /// </summary>
    IServiceProvider Services { get; }

    /// <summary>
    /// Starts listening on the configured addresses.
    /// </summary>
    void Start();

    /// <summary>
    /// Starts listening on the configured addresses.
    /// </summary>
    /// <param name="cancellationToken">Used to abort program start.</param>
    /// <returns>A <see cref="Task"/> that completes when the <see cref="IWebHost"/> starts.</returns>
    Task StartAsync(CancellationToken cancellationToken = default);

    /// <summary>

Review comment (suggestion):
```suggestion
[Obsolete("IWebHost is obsolete. Use IHost instead. For more information, visit https://aka.ms/aspnet/deprecate/008.", DiagnosticId = "ASPDEPR008")]
```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http.Features;

namespace Microsoft.AspNetCore.Hosting;

/// <summary>
/// Represents a configured web host.
/// </summary>
[Obsolete("IWebHost is obsolete. Use IHost instead. For more information, visit https://aka.ms/aspnet/deprecate/008.", DiagnosticId = "ASPDEPR008")]
public interface IWebHost : IDisposable
{
    /// <summary>
    /// The <see cref="IFeatureCollection"/> exposed by the configured server.
    /// </summary>
    IFeatureCollection ServerFeatures { get; }

    /// <summary>
    /// The <see cref="IServiceProvider"/> for the host.
    /// </summary>
    IServiceProvider Services { get; }

    /// <summary>
    /// Starts listening on the configured addresses.
    /// </summary>
    void Start();

    /// <summary>
    /// Starts listening on the configured addresses.
    /// </summary>
    /// <param name="cancellationToken">Used to abort program start.</param>
    /// <returns>A <see cref="Task"/> that completes when the <see cref="IWebHost"/> starts.</returns>
    Task StartAsync(CancellationToken cancellationToken = default);

    /// <summary>
Repository: dotnet/aspnetcore
File: src/Shared/Buffers.MemoryPool/DefaultMemoryPoolFactory.cs (C#)

Code before:
    {
        _metrics = metrics;
        _logger = logger;
        _timer = new PeriodicTimer(PinnedBlockMemoryPool.DefaultEvictionDelay);
        _timerTask = Task.Run(async () =>
        {
            try
            {
                while (await _timer.WaitForNextTickAsync())
                {
                    foreach (var pool in _pools.Keys)
                    {
                        pool.PerformEviction();
                    }
                }
            }
            catch (Exception ex)
            {
                _logger?.LogCritical(ex, "Error while evicting memory from pools.");
            }
        });
    }

    public MemoryPool<byte> Create(MemoryPoolOptions options)
    {
        var pool = new PinnedBlockMemoryPool(options?.Owner, _metrics, _logger);

        _pools.TryAdd(pool, true);

        pool.OnPoolDisposed(static (state, self) =>
        {
            ((ConcurrentDictionary<PinnedBlockMemoryPool, bool>)state!).TryRemove(self, out _);
        }, _pools);

        return pool;
    }

    public async ValueTask DisposeAsync()
    {
        _timer.Dispose();
        await _timerTask;
    }
}


Review comment (suggestion):
The code uses `options?.Owner` which could result in passing null to the constructor even when options is not null but Owner is null. This should be `options.Owner` since the parameter is marked as non-nullable in the method signature.
```suggestion
        if (options.Owner == null)
        {
            throw new ArgumentNullException(nameof(options.Owner), "Owner cannot be null.");
        }
        var pool = new PinnedBlockMemoryPool(options.Owner, _metrics, _logger);
```

Code after:
    {
        _metrics = metrics;
        _logger = logger;
        _timer = new PeriodicTimer(PinnedBlockMemoryPool.DefaultEvictionDelay);
        _timerTask = Task.Run(async () =>
        {
            try
            {
                while (await _timer.WaitForNextTickAsync())
                {
                    foreach (var pool in _pools.Keys)
                    {
                        pool.PerformEviction();
                    }
                }
            }
            catch (Exception ex)
            {
                _logger?.LogCritical(ex, "Error while evicting memory from pools.");
            }
        });
    }

    public MemoryPool<byte> Create(MemoryPoolOptions? options = null)
    {
        var pool = new PinnedBlockMemoryPool(options?.Owner, _metrics, _logger);

        _pools.TryAdd(pool, true);

        pool.OnPoolDisposed(static (state, self) =>
        {
            ((ConcurrentDictionary<PinnedBlockMemoryPool, bool>)state!).TryRemove(self, out _);
        }, _pools);

        return pool;
    }

    public async ValueTask DisposeAsync()
    {
        _timer.Dispose();
        await _timerTask;
    }
}
Repository: dotnet/aspnetcore
File: src/Servers/Connections.Abstractions/src/MemoryPoolOptions.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Connections;

/// <summary>
/// Options for configuring a memory pool.
/// </summary>
public sealed class MemoryPoolOptions
{
    /// <summary>
    /// Gets or sets the owner of the memory pool. This is used for logging and diagnostics purposes.
    /// </summary>
    public string? Owner { get; set; }
}


Review comment (suggestion):
I was thinking of making this non-sealed so users could provide their own superset options type and cast inside their pool implementations to support custom settings. e.g. if we wanted a custom PinnedObjectPoolOptions type that had an option for how often to try evicting.

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Connections;

/// <summary>
/// Options for configuring a memory pool.
/// </summary>
public class MemoryPoolOptions
{
    /// <summary>
    /// Gets or sets the owner of the memory pool. This is used for logging and diagnostics purposes.
    /// </summary>
    public string? Owner { get; set; }
}
Repository: dotnet/aspnetcore
File: src/OpenApi/sample/Controllers/TestController.cs (C#)

Code before:
    {
        return Ok(todo);
    }

    [HttpGet]
    [Route("/getcultureinvariant")]
    public Ok<CurrentWeather> GetCurrentWeather()
    {
        return TypedResults.Ok(new CurrentWeather(1.0f));
    }

    [Route("/nohttpmethod")]
    public ActionResult<CurrentWeather> NoHttpMethod()
        => Ok(new CurrentWeather(-100));

    [HttpQuery] // See https://github.com/dotnet/aspnetcore/issues/61089
    [Route("/query")]
    public ActionResult<CurrentWeather> HttpQueryMethod()
        => Ok(new CurrentWeather(0));

    [HttpFoo] // See https://github.com/dotnet/aspnetcore/issues/60914
    [Route("/unsupported")]
    public ActionResult<CurrentWeather> UnsupportedHttpMethod()
        => Ok(new CurrentWeather(100));

    public class HttpQuery() : HttpMethodAttribute(["QUERY"]);

    public class HttpFoo() : HttpMethodAttribute(["FOO"]);

    public class RouteParamsContainer
    {
        [FromRoute]
        public int Id { get; set; }

        [FromRoute]
        [MinLength(5)]
        [UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode", Justification = "MinLengthAttribute works without reflection on string properties.")]
        public string? Name { get; set; }
    }

    public record MvcTodo(string Title, string Description, bool IsCompleted);

    public record CurrentWeather([property: Range(-100.5f, 100.5f)] float Temperature = 0.1f);
}


Review comment (suggestion):
Insert a newline before the opening curly brace. According to the coding guidelines, there should be a newline before opening braces of code blocks.

Code after:
    {
        return Ok(todo);
    }

    [HttpGet]
    [Route("/getcultureinvariant")]
    public Ok<CurrentWeather> GetCurrentWeather()
    {
        return TypedResults.Ok(new CurrentWeather(1.0f));
    }

    [Route("/nohttpmethod")]
    public ActionResult<CurrentWeather> NoHttpMethod()
        => Ok(new CurrentWeather(-100));

    [HttpQuery]
    [Route("/query")]
    public ActionResult<CurrentWeather> HttpQueryMethod()
        => Ok(new CurrentWeather(0));

    [HttpFoo] // See https://github.com/dotnet/aspnetcore/issues/60914
    [Route("/unsupported")]
    public ActionResult<CurrentWeather> UnsupportedHttpMethod()
        => Ok(new CurrentWeather(100));

    public class HttpQuery() : HttpMethodAttribute(["QUERY"]);

    public class HttpFoo() : HttpMethodAttribute(["FOO"]);

    public class RouteParamsContainer
    {
        [FromRoute]
        public int Id { get; set; }

        [FromRoute]
        [MinLength(5)]
        [UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode", Justification = "MinLengthAttribute works without reflection on string properties.")]
        public string? Name { get; set; }
    }

    public record MvcTodo(string Title, string Description, bool IsCompleted);

    public record CurrentWeather([property: Range(-100.5f, 100.5f)] float Temperature = 0.1f);
}
Repository: dotnet/aspnetcore
File: src/OpenApi/sample/Controllers/TestController.cs (C#)

Code before:
    }

    [HttpGet]
    [Route("/getcultureinvariant")]
    public Ok<CurrentWeather> GetCurrentWeather()
    {
        return TypedResults.Ok(new CurrentWeather(1.0f));
    }

    [Route("/nohttpmethod")]
    public ActionResult<CurrentWeather> NoHttpMethod()
        => Ok(new CurrentWeather(-100));

    [HttpQuery] // See https://github.com/dotnet/aspnetcore/issues/61089
    [Route("/query")]
    public ActionResult<CurrentWeather> HttpQueryMethod()
        => Ok(new CurrentWeather(0));

    [HttpFoo] // See https://github.com/dotnet/aspnetcore/issues/60914
    [Route("/unsupported")]
    public ActionResult<CurrentWeather> UnsupportedHttpMethod()
        => Ok(new CurrentWeather(100));

    public class HttpQuery() : HttpMethodAttribute(["QUERY"]);

    public class HttpFoo() : HttpMethodAttribute(["FOO"]);

    public class RouteParamsContainer
    {
        [FromRoute]
        public int Id { get; set; }

        [FromRoute]
        [MinLength(5)]
        [UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode", Justification = "MinLengthAttribute works without reflection on string properties.")]
        public string? Name { get; set; }
    }

    public record MvcTodo(string Title, string Description, bool IsCompleted);

    public record CurrentWeather([property: Range(-100.5f, 100.5f)] float Temperature = 0.1f);
}


Review comment (suggestion):
Insert a newline before the opening curly brace. According to the coding guidelines, there should be a newline before opening braces of code blocks.

Code after:
    }

    [HttpGet]
    [Route("/getcultureinvariant")]
    public Ok<CurrentWeather> GetCurrentWeather()
    {
        return TypedResults.Ok(new CurrentWeather(1.0f));
    }

    [Route("/nohttpmethod")]
    public ActionResult<CurrentWeather> NoHttpMethod()
        => Ok(new CurrentWeather(-100));

    [HttpQuery]
    [Route("/query")]
    public ActionResult<CurrentWeather> HttpQueryMethod()
        => Ok(new CurrentWeather(0));

    [HttpFoo] // See https://github.com/dotnet/aspnetcore/issues/60914
    [Route("/unsupported")]
    public ActionResult<CurrentWeather> UnsupportedHttpMethod()
        => Ok(new CurrentWeather(100));

    public class HttpQuery() : HttpMethodAttribute(["QUERY"]);

    public class HttpFoo() : HttpMethodAttribute(["FOO"]);

    public class RouteParamsContainer
    {
        [FromRoute]
        public int Id { get; set; }

        [FromRoute]
        [MinLength(5)]
        [UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode", Justification = "MinLengthAttribute works without reflection on string properties.")]
        public string? Name { get; set; }
    }

    public record MvcTodo(string Title, string Description, bool IsCompleted);

    public record CurrentWeather([property: Range(-100.5f, 100.5f)] float Temperature = 0.1f);
}
Repository: dotnet/aspnetcore
File: src/Components/test/E2ETest/Infrastructure/WebDriverExtensions/WebDriverExtensions.cs (C#)

Code before:
            }
            catch (StaleElementReferenceException)
            {
                // Retry finding the element
                return false;
            }
        });
    }

    public static long GetElementPositionWithRetry(this IWebDriver browser, string elementId, int retryCount = 3, int delayBetweenRetriesMs = 100)
    {
        string log = "";

        for (int i = 0; i < retryCount; i++)
        {
            try
            {
                browser.WaitForElementToBeVisible(By.Id(elementId));
                var element = browser.FindElement(By.Id(elementId));
                var elementLocation = element.Location.Y;

                // Get scroll position using JavaScript (this is less likely to fail than element positioning)
                var jsExecutor = (IJavaScriptExecutor)browser;
                var scrollY = (long)jsExecutor.ExecuteScript("return window.scrollY");

                return elementLocation + scrollY;
            }
            catch (Exception ex)
            {
                log += $"Attempt {i + 1}: - {ex.Message}. ";
            }

            if (i < retryCount - 1)
            {
                Thread.Sleep(delayBetweenRetriesMs);
            }
        }

        throw new Exception($"Failed to get position for element '{elementId}' after {retryCount} retries. Debug log: {log}");
    }
}


Review comment (bug):
Is this correct?
```
element.Location.Y (in Selenium) already gives you the Y position of the element relative to the top of the document, not the viewport.
window.scrollY is the number of pixels the document has been scrolled vertically.
Adding them together double-counts the scroll, resulting in an incorrect value.
```

Code after:
            }
            catch (StaleElementReferenceException)
            {
                // Retry finding the element
                return false;
            }
        });
    }

    public static long GetElementPositionWithRetry(this IWebDriver browser, string elementId, int retryCount = 3, int delayBetweenRetriesMs = 100)
    {
        string log = "";

        for (int i = 0; i < retryCount; i++)
        {
            try
            {
                browser.WaitForElementToBeVisible(By.Id(elementId));
                var element = browser.FindElement(By.Id(elementId));
                return element.Location.Y;
            }
            catch (Exception ex)
            {
                log += $"Attempt {i + 1}: - {ex.Message}. ";
            }

            if (i < retryCount - 1)
            {
                Thread.Sleep(delayBetweenRetriesMs);
            }
        }

        throw new Exception($"Failed to get position for element '{elementId}' after {retryCount} retries. Debug log: {log}");
    }
}
Repository: dotnet/aspnetcore
File: src/SignalR/clients/ts/signalr/src/HubConnection.ts (TypeScript)

Code before:
        };
    }

    private _createCompletionMessage(id: string, error?: any, result?: any): CompletionMessage {
        if (error) {
            return {
                error,
                invocationId: id,
                type: MessageType.Completion,
            };
        }

        return {
            invocationId: id,
            result,
            type: MessageType.Completion,
        };
    }

    private _createCloseMessage(): CloseMessage {
        return { type: MessageType.Close };
    }

    private _trySendPingMessage(): void {
        try {
            this._sendMessage(this._cachedPingMessage);
        } catch {
            // We don't care about the error. It should be seen elsewhere in the client.
            // The connection is probably in a bad or closed state now, cleanup the timer so it stops triggering
            this._cleanupPingTimer();
        }
    }
}


Review comment (performance):
```suggestion
    private _trySendPingMessage(): Promise<void> {
        try {
            await this._sendMessage(this._cachedPingMessage);
```

Code after:
            type: MessageType.StreamItem,
        };
    }

    private _createCompletionMessage(id: string, error?: any, result?: any): CompletionMessage {
        if (error) {
            return {
                error,
                invocationId: id,
                type: MessageType.Completion,
            };
        }

        return {
            invocationId: id,
            result,
            type: MessageType.Completion,
        };
    }

    private _createCloseMessage(): CloseMessage {
        return { type: MessageType.Close };
    }

    private async _trySendPingMessage(): Promise<void> {
        try {
            await this._sendMessage(this._cachedPingMessage);
        } catch {
            // We don't care about the error. It should be seen elsewhere in the client.
            // The connection is probably in a bad or closed state now, cleanup the timer so it stops triggering
            this._cleanupPingTimer();
        }
    }
}