[dotnet] [bidi] Properly return shared buffer when disposing websocket

[nvborisenko]

User description

🔗 Related Issues

Fixes #16801

💥 What does this PR do?

Introduces Dispose(bool disposing) method to make sure shared buffer returned back to the pool only once.

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Implements proper IDisposable pattern with Dispose(bool) method

• Ensures shared buffer returned to pool only once during disposal

• Prevents double-disposal of managed resources

• Seals class and nullifies buffer reference after return

---

Diagram Walkthrough

flowchart LR
  A["WebSocketTransport"] -->|"implements"| B["IDisposable pattern"]
  B -->|"Dispose method"| C["calls Dispose(bool)"]
  C -->|"disposing=true"| D["dispose managed resources"]
  C -->|"disposing=false"| E["skip managed resources"]
  D -->|"then"| F["return buffer to pool once"]
  E -->|"then"| F
  F -->|"nullify buffer"| G["prevent double-return"]

Loading

File Walkthrough

Relevant files

Bug fix

WebSocketTransport.cs Implement standard IDisposable pattern with proper cleanup dotnet/src/webdriver/BiDi/WebSocketTransport.cs Changed class from unsealed to sealed to prevent inheritance issues Changed _receiveBuffer from readonly to mutable field to allow nullification Refactored Dispose() to call new Dispose(bool disposing) method with GC.SuppressFinalize() Implemented Dispose(bool disposing) pattern to separate managed and unmanaged resource cleanup Moved buffer return logic to Dispose(bool) and nullified buffer after return to prevent double-disposal Updated finalizer to call Dispose(false) instead of directly calling ReleaseBuffer() +22/-14

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Dispose race condition: The new Dispose(bool disposing) uses a non-atomic _disposed check and a non-atomic _receiveBuffer nullification, which can still allow concurrent Dispose() calls to double-return the shared buffer to the pool. Referred Code private void Dispose(bool disposing) { if (_disposed) { return; } if (disposing) { _webSocket.Dispose(); _sharedMemoryStream.Dispose(); _socketSendSemaphoreSlim.Dispose(); } if (_receiveBuffer is not null) { ArrayPool<byte>.Shared.Return(_receiveBuffer); _receiveBuffer = null!; } ... (clipped 2 lines) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix race condition in Dispose Fix a race condition in the Dispose method by using Interlocked.CompareExchange to ensure the disposal logic runs only once, preventing potential memory corruption. dotnet/src/webdriver/BiDi/WebSocketTransport.cs [91-126] -private bool _disposed; +private int _disposed; public void Dispose() { Dispose(true); GC.SuppressFinalize(this); } ~WebSocketTransport() { Dispose(false); } private void Dispose(bool disposing) { - if (_disposed) + if (Interlocked.CompareExchange(ref _disposed, 1, 0) != 0) { return; } if (disposing) { _webSocket.Dispose(); _sharedMemoryStream.Dispose(); _socketSendSemaphoreSlim.Dispose(); } if (_receiveBuffer is not null) { ArrayPool<byte>.Shared.Return(_receiveBuffer); _receiveBuffer = null!; } - - _disposed = true; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical race condition in the Dispose implementation that could lead to memory corruption and provides the standard, lock-free solution using Interlocked.CompareExchange.	High
	Always return buffer on dispose Improve exception safety in the Dispose method by using a try/finally block to guarantee the rented buffer is always returned to the pool, even if an error occurs. dotnet/src/webdriver/BiDi/WebSocketTransport.cs [104-126] private void Dispose(bool disposing) { if (_disposed) { return; } - if (disposing) + try { - _webSocket.Dispose(); - _sharedMemoryStream.Dispose(); - _socketSendSemaphoreSlim.Dispose(); + if (disposing) + { + _webSocket.Dispose(); + _sharedMemoryStream.Dispose(); + _socketSendSemaphoreSlim.Dispose(); + } } - - if (_receiveBuffer is not null) + finally { - ArrayPool<byte>.Shared.Return(_receiveBuffer); - - _receiveBuffer = null!; + if (_receiveBuffer is not null) + { + ArrayPool<byte>.Shared.Return(_receiveBuffer); + _receiveBuffer = null!; + } + _disposed = true; } - - _disposed = true; } Apply / Chat Suggestion importance[1-10]: 7 __ Why: This suggestion correctly improves the exception safety of the Dispose method, preventing a potential buffer leak by using a try/finally block, which is a robust programming practice.	Medium
General	Prevent operations after dispose Add a check at the beginning of public methods to throw an ObjectDisposedException if the object has already been disposed. dotnet/src/webdriver/BiDi/WebSocketTransport.cs [41-44] public async Task ConnectAsync(CancellationToken cancellationToken) { + if (_disposed) + { + throw new ObjectDisposedException(nameof(WebSocketTransport)); + } await _webSocket.ConnectAsync(_uri, cancellationToken).ConfigureAwait(false); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that public methods should check if the object has been disposed. This is a standard practice for disposable types and improves the class's robustness.	Low
More

[Piedone]

Thank you!

[nvborisenko]

@Piedone you can test it when nightly package (GitHub) will be released. Interesting whether it is proper fix.

[Piedone]

I'll check, yep.

[nvborisenko]

@RenderMichael @YevgeniyShunevych what if we rent this buffer in the method, and return immediately? At least in this way we can remove finalizer, which brings more complexity.