-
Notifications
You must be signed in to change notification settings - Fork 470
Add backpressure when rapidly creating new stateful Streamable HTTP sessions without closing them #677
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
Add backpressure when rapidly creating new stateful Streamable HTTP sessions without closing them #677
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,27 @@ | ||
using ModelContextProtocol.AspNetCore.Stateless; | ||
using ModelContextProtocol.Protocol; | ||
using ModelContextProtocol.Server; | ||
using System.Diagnostics; | ||
using System.Security.Claims; | ||
using System.Threading; | ||
|
||
namespace ModelContextProtocol.AspNetCore; | ||
|
||
internal sealed class HttpMcpSession<TTransport>( | ||
string sessionId, | ||
TTransport transport, | ||
UserIdClaim? userId, | ||
TimeProvider timeProvider) : IAsyncDisposable | ||
TimeProvider timeProvider, | ||
SemaphoreSlim? idleSessionSemaphore = null) : IAsyncDisposable | ||
where TTransport : ITransport | ||
{ | ||
private int _referenceCount; | ||
private int _getRequestStarted; | ||
private CancellationTokenSource _disposeCts = new(); | ||
private bool _isDisposed; | ||
|
||
private readonly SemaphoreSlim? _idleSessionSemaphore = idleSessionSemaphore; | ||
private readonly CancellationTokenSource _disposeCts = new(); | ||
private readonly object _referenceCountLock = new(); | ||
|
||
public string Id { get; } = sessionId; | ||
public TTransport Transport { get; } = transport; | ||
|
@@ -30,16 +37,43 @@ internal sealed class HttpMcpSession<TTransport>( | |
public IMcpServer? Server { get; set; } | ||
public Task? ServerRunTask { get; set; } | ||
|
||
public IDisposable AcquireReference() | ||
public IAsyncDisposable AcquireReference() | ||
{ | ||
Interlocked.Increment(ref _referenceCount); | ||
// We don't do idle tracking for stateless sessions, so we don't need to acquire a reference. | ||
if (_idleSessionSemaphore is null) | ||
{ | ||
return new NoopDisposable(); | ||
} | ||
|
||
lock (_referenceCountLock) | ||
{ | ||
if (!_isDisposed && ++_referenceCount == 1) | ||
halter73 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
// Non-idle sessions should not prevent session creation. | ||
_idleSessionSemaphore.Release(); | ||
} | ||
} | ||
|
||
return new UnreferenceDisposable(this); | ||
} | ||
|
||
public bool TryStartGetRequest() => Interlocked.Exchange(ref _getRequestStarted, 1) == 0; | ||
|
||
public async ValueTask DisposeAsync() | ||
{ | ||
bool shouldReleaseIdleSessionSemaphore; | ||
|
||
lock (_referenceCountLock) | ||
{ | ||
if (_isDisposed) | ||
{ | ||
return; | ||
} | ||
|
||
_isDisposed = true; | ||
shouldReleaseIdleSessionSemaphore = _referenceCount == 0; | ||
} | ||
|
||
try | ||
{ | ||
await _disposeCts.CancelAsync(); | ||
|
@@ -65,21 +99,45 @@ public async ValueTask DisposeAsync() | |
{ | ||
await Transport.DisposeAsync(); | ||
_disposeCts.Dispose(); | ||
|
||
// If the session was disposed while it was inactive, we need to release the semaphore | ||
// to allow new sessions to be created. | ||
if (_idleSessionSemaphore is not null && shouldReleaseIdleSessionSemaphore) | ||
{ | ||
_idleSessionSemaphore.Release(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
public bool HasSameUserId(ClaimsPrincipal user) | ||
=> UserIdClaim == StreamableHttpHandler.GetUserIdClaim(user); | ||
public bool HasSameUserId(ClaimsPrincipal user) => UserIdClaim == StreamableHttpHandler.GetUserIdClaim(user); | ||
|
||
private sealed class UnreferenceDisposable(HttpMcpSession<TTransport> session) : IDisposable | ||
private sealed class UnreferenceDisposable(HttpMcpSession<TTransport> session) : IAsyncDisposable | ||
{ | ||
public void Dispose() | ||
public async ValueTask DisposeAsync() | ||
{ | ||
if (Interlocked.Decrement(ref session._referenceCount) == 0) | ||
Debug.Assert(session._idleSessionSemaphore is not null, "Only StreamableHttpHandler should call AcquireReference."); | ||
|
||
bool shouldMarkSessionIdle; | ||
|
||
lock (session._referenceCountLock) | ||
{ | ||
shouldMarkSessionIdle = !session._isDisposed && --session._referenceCount == 0; | ||
} | ||
|
||
if (shouldMarkSessionIdle) | ||
{ | ||
session.LastActivityTicks = session.TimeProvider.GetTimestamp(); | ||
|
||
// Acquire semaphore when session becomes inactive (reference count goes to 0) to slow | ||
// down session creation until idle sessions are disposed by the background service. | ||
await session._idleSessionSemaphore.WaitAsync(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this wait forever since there is no cancellationToken? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory no, since the session is already marked as idle by virtue of the reference count going to zero, the corresponding call to Release() to unblock this should not be inhibited. For this to wait, that means we are at least at 110% of the MaxIdleSessionCount. Either idle tracking background service will eventually dispose enough sessions to get it below 110% and call Release() or another request will reactivate the session and call Release(). And any sessions waiting for this call to WaitAsync() to complete are already eligible for pruning if they haven't been reactivated. |
||
} | ||
} | ||
} | ||
|
||
private sealed class NoopDisposable : IAsyncDisposable | ||
{ | ||
public ValueTask DisposeAsync() => ValueTask.CompletedTask; | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.