Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for ObjectDisposedException in PodeRequest.Receive During SSL/TLS Operations #1460

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

mdaneri
Copy link
Contributor

@mdaneri mdaneri commented Nov 29, 2024

Pull Request: Fix for ObjectDisposedException in PodeRequest.Receive During SSL/TLS Operations

Summary

This pull request addresses #1459
Intermittent ObjectDisposedException that occurs in the PodeRequest.Receive method during SSL/TLS operations in Pode. The issue arises due to premature disposal or concurrent access of the SslStream instance when handling HTTPS requests under high concurrency.

Problem

The following error was observed when running a Pode server with an HTTPS endpoint under load:

[Error] ObjectDisposedException: Cannot access a disposed object.
Object name: 'SslStream'.
   at System.Net.Security.SslState.CheckThrow(Boolean authSuccessCheck, Boolean shutdownCheck)
   at System.Net.Security.SslState.get_SecureStream()
   at System.Net.Security.SslStream.EndRead(IAsyncResult asyncResult)
   at System.Threading.Tasks.TaskFactory`1.FromAsyncTrimPromise`1.Complete(TInstance thisRef, Func`3 endMethod, IAsyncResult asyncResult, Boolean requiresSynchronization)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Pode.PodeRequest.<Receive>d__83.MoveNext()

The issue is caused by:

  1. Premature Disposal: The SslStream instance is disposed of while operations are still in progress.
  2. Concurrent Access: Multiple threads attempting to access the SslStream without proper synchronization.

Changes Made

  1. Improved Thread Safety:

    • Added additional checks to ensure that all operations on SslStream are thread-safe, using SemaphoreSlim to manage concurrency.
  2. Enhanced Disposal Logic:

    • Updated the Dispose method in PodeRequest to ensure that the SslStream is not disposed prematurely.
    • Introduced IsDisposed and IsOpen state checks to prevent operations on already-disposed streams.
  3. Error Logging:

    • Adjusted error logging levels for expected exceptions (ObjectDisposedException and IOException) to avoid alarming users unnecessarily. These exceptions are now logged at the Verbose level instead of Error.
  4. Buffer Management:

    • Optimized buffer handling to prevent unnecessary reinitializations and ensure reuse during stream operations.
  5. Stream Cleanup:

    • Ensured proper handling and disposal of streams, including defensive null checks and resetting state variables like InputStream.

Results

  • The ObjectDisposedException no longer occurs under high-concurrency scenarios.
  • The Pode server handles concurrent HTTPS requests more reliably without resource contention or premature disposal.

Impact

  • Improved stability and reliability for HTTPS endpoints in Pode.
  • Resolved a critical issue affecting high-concurrency scenarios, ensuring seamless handling of SSL/TLS operations.

Backward Compatibility

  • These changes are fully backward-compatible with existing Pode functionality.

return false;
}

await StreamLock.WaitAsync(cancellationToken);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semaphore's aren't needed, because of the way the Receiving works this method will only ever been called one-at-a-time for one Request object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes you are right. I added it when I was trying to fix the reported issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to reinstate it because otherwise everything fails

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, I was more referring that the StreamLock variable entirely can go - because of how code will drop into this method, Semaphores shouldn't be required, and the locking will just add overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot remove the Semaphores. there are multiple receive calls I presume.

src/Listener/PodeRequest.cs Outdated Show resolved Hide resolved
src/Listener/PodeRequest.cs Outdated Show resolved Hide resolved
src/Listener/PodeRequest.cs Show resolved Hide resolved
src/Listener/PodeHttpRequest.cs Outdated Show resolved Hide resolved
@Badgerati Badgerati linked an issue Dec 10, 2024 that may be closed by this pull request
@Badgerati Badgerati added this to the 2.12.0 milestone Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ObjectDisposedException in PodeRequest During SslStream Operation
2 participants