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

Mailkit upgrade #8746

Open
wants to merge 3 commits into
base: 1.10.x
Choose a base branch
from

Conversation

MatteoPiovanelli-Laser
Copy link
Contributor

Since upgrading to mailkit, we've been having a few issues.

Specifically, it looked like the client (i.e. Orchard, where Mailkit is) didn't wait for the smtp server to acknowledge reception of message data. Often, we would get this error on our SMTP servers:

Client socket is disconnected! Disconnect exception encountered: False, IsDisconnected: True, This message will be rejected.
[...]
This issue is caused by the sending server not waiting for the DATA OK command

We tried to upgrade the version of the NuGet package, but there were several issues with its dependencies (jstedfast/MailKit#1462, jstedfast/MimeKit#872, dotnet/sdk#31147) so we were only able to advance it to 3.4.2.

We then changed the code a bit, to explicitly disconnect from the STMP server and dispose of the smtpclient after sending each email. This seems to have improved things.
We couldn't replicate the error 100% of the time, but so far in our tests with this code it hasn't occurred yet.

@MatteoPiovanelli-Laser
Copy link
Contributor Author

The thing the compile check is failing on is not something I changed in this PR, I think?

@sebastienros
Copy link
Member

Try to rerun the build, or maybe check if there is a floating version somehow that would fail without any change.

@MatteoPiovanelli-Laser
Copy link
Contributor Author

if it still doesn't build, I'll "recreate" the PR on step at a time.

@BenedekFarkas
Copy link
Member

BenedekFarkas commented Mar 11, 2024

System.Net.Http is unfortunately a well-known troublemaker, although the relevant threads I found about it all state that it should stop causing problems with .NET 4.8 (or even 4.7.2). I've seen the same/similar errors before (some combination of Upgrade, Orchard.Workflows and Orchard.Email were involved), but managed to find a stable (at least it looked stable at the time) configuration.

It looks like that MimeKit (or one of its dependencies) causes a conflict again, maybe it requires a more recent version of System.Net.Http, but we restrict it to 4.2.0.0.

I'm working on this too, but in the meanwhile, if you isolate any specific change/version that causes the warning (error), please let me know. This might help: https://nickcraver.com/blog/2020/02/11/binding-redirects/#the-fix-if-youre-using-net-framework

@BenedekFarkas
Copy link
Member

I managed to get view compilation pass by upgrading Microsoft.CodeDom.Providers.DotNetCompilerPlatform to the latest version (4.1.0) on top of this PR (we're currently using 2.0.1, which is more than 5 years old).
I'm not 100% sure why, but version 2.0.1 actually ships with the .NET 4.6-version of System.Net.Http.dll. If I remove that package (and the associated configuration) from Orchard.Workflows, then Razor compilation passes fine.

Also tested the upgrade to 4.1.0 on current 1.10.x and dev. I'll submit a PR targeting 1.10.x for that soon.

@BenedekFarkas
Copy link
Member

Please merge the latest 1.10.x to fix the build error.

@MatteoPiovanelli-Laser
Copy link
Contributor Author

rebased on 1.10.x

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