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

Add Smtp SessionTimout #227

Merged
merged 12 commits into from
Sep 9, 2024
Merged

Add Smtp SessionTimout #227

merged 12 commits into from
Sep 9, 2024

Conversation

tinohager
Copy link
Contributor

Create a LinkedTokenSource in the SmtpSession with a Timeout

#226

@tinohager tinohager marked this pull request as ready for review August 30, 2024 09:26
/// </summary>
TimeSpan ReadTimeout { get; }
Copy link
Owner

Choose a reason for hiding this comment

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

A session timeout should be added on top of, not in replace of the read timeout.

The ReadTimeout should be more efficent as it can be handled at a lower level so should still apply to individual read operations.

If we want to also support a session timeout, the then it should be in addition.

There are also mail clients which could create long running connections/sessions, so a session timeout should probably only be enabled by choice, not by default.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, I think a SessionTimeout is something that should be handled at a higher level that the SmtpSession class. It probably belongs in the SmtpSessionManager... the Run method here takes in the global CancellationToken, but then it should probably create a linked token source and pass it down to the SmtpSession.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is, that ReadTimeout of the underlying NetworkStream does not affect ReadAsync which is being used by SmtpServer. It only affects synchronous reads.
#226

From the MS docs:

This property affects only synchronous reads performed by calling the Read method. This property does not affect asynchronous reads performed by calling the BeginRead or ReadAsync method.

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 can move the linked CancellationToken logic to the SmtpSessionManager and add the logic in that location, is this ok with you?

internal void Run(SmtpSessionContext sessionContext, CancellationToken cancellationToken)

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, just thinking about this a bit... so if we want to add the ability to cancel a session based on timeout, then potentially there might be other ways thats user want to cancel a session.. ie, maybe disk storage, or time of day, etc.

So in that sense, maybe we should allow the session timeout to be handled externally.

There is an SessionCreated event which allows the SessionContext to be modified outside of the core code, we could so something similar with a new event that allows the CancellationToken to be provided. The problem is that this even is probably actually called SessionCreated and the SessionCreated event we have now is really SessionStarted.

But say we had something like this

`
internal void Run(SmtpSessionContext sessionContext, CancellationToken cancellationToken)
{
var handle = new SmtpSessionHandle(new SmtpSession(sessionContext), sessionContext);
Add(handle);

var sessionCancellationToken = _smtpServer.OnSessionBegining(new SessionBeginingEventArgs(handle.SessionContext));

// create the linked token source here
...

handle.CompletionTask = RunAsync(handle, cancellationToken);
 ...

}
`

so the user could do

server.SessionBegining += (args) {
   args.CancellationToken = ...
}

thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I move the logic with the CancellationToken to the SmtpSessionManager. Do you then accept the PR? Or what is your wish as we continue here?

Choose a reason for hiding this comment

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

The ReadTimeout currently has no effect at all, as it is ignored by ReadAsync. @tinohager do you wish to control/cancel the session, or is it "enough" to get the read timeout to work. Which should then close the session, as of my observation.

Choose a reason for hiding this comment

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

and also query how many sessions are active and open

We added public int SessionsCount => _sessions.SessionsCount; to SmtpServer to get the current sessions count, which was a quick solution for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martinguenther I would like to have control over the open sessions and be able to close them manually, but this should not be part of this pull request.

Copy link
Owner

Choose a reason for hiding this comment

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

I think any manual control of the sessions should be handled by the CancellationToken to that specific session, so this is the functionality that we should expose.

Read-only information about the sessions can probably be exposed.

@tinohager tinohager requested a review from cosullivan September 4, 2024 18:44
@cosullivan cosullivan merged commit 67375e1 into cosullivan:master Sep 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants