-
-
Notifications
You must be signed in to change notification settings - Fork 166
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
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
17e8552
Add build script
tinohager e3792af
reactivate one unit test
tinohager 6c7f1fd
Update SmtpServerTests.cs
tinohager 32ce1fa
Reactivate all unit tests
tinohager 677a295
Update SmtpServerTests.cs
tinohager e88e38b
Merge branch 'cosullivan:master' into master
tinohager 92928a7
Merge branch 'cosullivan:master' into master
tinohager 85d75a9
Add Smtp session Timeout
tinohager ffe91bc
Add unit test for session timeout
tinohager 86972e7
namespace cleanup
tinohager 9bfd78d
Merge branch 'cosullivan:master' into master
tinohager 6bfa3fb
Move LinkedTokenSource to SmtpSessionHandle
tinohager File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
using System; | ||
using System.Net; | ||
using System.Net.Sockets; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
|
||
namespace SmtpServer.Tests | ||
{ | ||
internal class RawSmtpClient : IDisposable | ||
{ | ||
private readonly TcpClient _tcpClient; | ||
private NetworkStream _networkStream; | ||
|
||
internal RawSmtpClient(string host, int port) | ||
{ | ||
_tcpClient = new TcpClient(); | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
_networkStream?.Dispose(); | ||
_tcpClient.Dispose(); | ||
} | ||
|
||
internal async Task<bool> ConnectAsync() | ||
{ | ||
await _tcpClient.ConnectAsync(new IPEndPoint(IPAddress.Parse("127.0.0.1"), 9025)); | ||
_networkStream = _tcpClient.GetStream(); | ||
|
||
var greetingResponse = await WaitForDataAsync(); | ||
if (greetingResponse.StartsWith("220")) | ||
{ | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
internal async Task<string> SendCommandAsync(string command) | ||
{ | ||
var commandData = Encoding.UTF8.GetBytes($"{command}\r\n"); | ||
|
||
await _networkStream.WriteAsync(commandData, 0, commandData.Length); | ||
return await WaitForDataAsync(); | ||
} | ||
|
||
internal async Task SendDataAsync(string data) | ||
{ | ||
var mailData = Encoding.UTF8.GetBytes(data); | ||
|
||
await _networkStream.WriteAsync(mailData, 0, mailData.Length); | ||
} | ||
|
||
internal async Task<string> WaitForDataAsync() | ||
{ | ||
var buffer = new byte[1024]; | ||
int bytesRead; | ||
|
||
while ((bytesRead = await _networkStream.ReadAsync(buffer, 0, buffer.Length)) > 0) | ||
{ | ||
var receivedData = Encoding.UTF8.GetString(buffer, 0, bytesRead); | ||
return receivedData; | ||
} | ||
|
||
return null; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
using SmtpServer.Storage; | ||
using SmtpServer.Tests.Mocks; | ||
using System; | ||
using System.Diagnostics; | ||
using System.IO; | ||
using System.Net; | ||
using System.Security.Authentication; | ||
|
@@ -159,6 +160,79 @@ public void WillTimeoutWaitingForCommand() | |
} | ||
} | ||
|
||
[Fact] | ||
public async Task WillSessionTimeoutDuringMailDataTransmission() | ||
{ | ||
var sessionTimeout = TimeSpan.FromSeconds(5); | ||
var commandWaitTimeout = TimeSpan.FromSeconds(1); | ||
|
||
using var disposable = CreateServer( | ||
serverOptions => serverOptions.CommandWaitTimeout(commandWaitTimeout), | ||
endpointDefinition => endpointDefinition.SessionTimeout(sessionTimeout)); | ||
|
||
var stopwatch = new Stopwatch(); | ||
stopwatch.Start(); | ||
|
||
using var rawSmtpClient = new RawSmtpClient("127.0.0.1", 9025); | ||
await rawSmtpClient.ConnectAsync(); | ||
|
||
var response = await rawSmtpClient.SendCommandAsync("helo test"); | ||
if (!response.StartsWith("250")) | ||
{ | ||
Assert.Fail("helo command not successful"); | ||
} | ||
|
||
response = await rawSmtpClient.SendCommandAsync("mail from:<[email protected]>"); | ||
if (!response.StartsWith("250")) | ||
{ | ||
Assert.Fail("mail from command not successful"); | ||
} | ||
|
||
response = await rawSmtpClient.SendCommandAsync("rcpt to:<[email protected]>"); | ||
if (!response.StartsWith("250")) | ||
{ | ||
Assert.Fail("rcpt to command not successful"); | ||
} | ||
|
||
response = await rawSmtpClient.SendCommandAsync("data"); | ||
if (!response.StartsWith("354")) | ||
{ | ||
Assert.Fail("data command not successful"); | ||
} | ||
|
||
string smtpResponse = null; | ||
|
||
_ = Task.Run (async() => | ||
{ | ||
smtpResponse = await rawSmtpClient.WaitForDataAsync(); | ||
}); | ||
|
||
var isSessionCancelled = false; | ||
|
||
try | ||
{ | ||
for (var i = 0; i < 1000; i++) | ||
{ | ||
await rawSmtpClient.SendDataAsync("some text part "); | ||
await Task.Delay(100); | ||
} | ||
} | ||
catch (IOException) | ||
{ | ||
isSessionCancelled = true; | ||
stopwatch.Stop(); | ||
} | ||
catch (Exception exception) | ||
{ | ||
Assert.Fail($"Wrong exception type {exception.GetType()}"); | ||
} | ||
|
||
Assert.True(isSessionCancelled, "Smtp session is not cancelled"); | ||
Assert.Equal("554 \r\n221 The session has be cancelled.\r\n", smtpResponse); | ||
|
||
Assert.True(stopwatch.Elapsed > sessionTimeout, "SessionTimeout not reached"); | ||
} | ||
|
||
[Fact] | ||
public void CanReturnSmtpResponseException_DoesNotQuit() | ||
{ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the MS docs:
There was a problem hiding this comment.
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?
SmtpServer/Src/SmtpServer/SmtpSessionManager.cs
Line 20 in cffd7df
There was a problem hiding this comment.
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);
}
`
so the user could do
thoughts?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 byReadAsync
. @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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added
public int SessionsCount => _sessions.SessionsCount;
toSmtpServer
to get the current sessions count, which was a quick solution for us.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.