Repository navigation
Conversation
7137e11 to
e39287e
Compare
link2xt
changed the title
|
|
||
| let handlers = SmtpConnectionHandlers { | ||
| connection: Smtp::new(), | ||
| connection: Smtp::new(context).await?, |
There was a problem hiding this comment.
One of the commit messages says:
refactor: load ProxyConfig once, when Smtp is created
Changing proxy settings requires restarting I/O anyway
and at least Delta Chat for Android does it,
soSmtpis recreated from scratch.
TODO: Confirm that other UIs than Android also restart IO when proxy config changes. They should, but it's unclear whether this was communicated correctly.
| let (sender, receiver) = mpsc::channel(2); | ||
| let actor = Connector::new(context.clone(), proxy_config.clone(), lp)?; | ||
| let cloned_cancellation_token = cancellation_token.clone(); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Could use JoinSet rather than plain spawn, then we don't need the extra logic of cancellation token and DropGuard
There was a problem hiding this comment.
I did it differently, have now moved CancellationToken all the way up to start_io() and moved cancellation out of smtp_loop in this PR already. Scheduler constructs the cancellation token when I/O is started and passes it as an argument to Smtp::new() so the token is cloned into all connection actor tasks started by Smtp::new(). When I/O is stopped, it cancels the token stored in SmtpConnectionState.
In the tests Smtp::with_connection_actors() is used and it does not require the cancellation token. And because there is no DropGuard or JoinSet inside Smtp structure, it does not have to create unused drop guard or empty JoinSet that is useless for the tests because there are no actual actors that can be cancelled.
|
|
||
| /// SMTP actor that maintains connection to a single transport. | ||
| #[derive(Debug)] | ||
| pub(crate) struct Connector { |
There was a problem hiding this comment.
The names are quite generic (Connector, Connection, ConnectorMessage), maybe we can come up with a better name? Not sure yet what it might be, though:
Connectoris an actor that contains aConnection, and has functionsrun()andhandle_message(ConnectorMessage).ConnectorMessageis an enum that is sent via a channel to theConnector. It has the variantsConnectandSendMail.Connectioncontains anasync_smtp::SmtpTransport, which contains the actual stream to send to the server.Smtpis the SMTP loop.
After talking to @link2xt, these are some renamings:
Connector->ConnectionActor, then make sure that it's not exported outside of thesmtpmodule because the name doesn't say that it's about smtpConnectorMessage->ConnectionActorMessage(that's quite long, but because we're a messenger, just calling itMessagewould be confusing)- In another PR (because this PR is quite big already, and these existed before this PR):
Smtp->SmtpLoop(in another PR); smtp_loop can be moved outside of scheduler and becomeSmtp::runConnection.transport->Connection.innerbecausetransportmeans something different nowSmtpConnectionHandlerscan be removed, the interrupt can be passed toSmtpLoop::run()
There was a problem hiding this comment.
I ranamed connector -> connection_actor, Connector -> ConnectionActor, ConnectorMessage -> ConnectionActorMessage.
Also removed SmtpConnectionHandlers already so the test does not have to create it.
f18046e to
1ccedad
Compare
link2xt
changed the title
c33778e to
92df802
Compare
|
Added basic test for chunking to the end of |
c47bd26 to
79512f8
Compare
| let callback = expect_connect(&mut alice_actors_rx[0]).await?; | ||
| callback.send(Ok(())).unwrap(); | ||
|
|
||
| if i == 7 { |
There was a problem hiding this comment.
Retry limit logic is a bit broken: message is only marked as "retry limit exceeded" after connecting and right before actually trying to send. I made a test for what is currently implemented for now, but we can actually fail the message one step before and don't try to connect just to say that we are not going to send the message right after connecting.
79512f8 to
ce120c5
Compare
dee7e97 to
4ff6562
Compare
|
|
||
| /// Temporary error, e.g. temporary SMTP error | ||
| /// or connection failure during message upload. | ||
| TemporaryFailure(Error), |
There was a problem hiding this comment.
This temporary failure includes both SMTP temporary failure and network errors such as lost connections during mail upload.
We have already increased retry counter for network errors before, because we increase retry counter before even trying to send the message. With this PR we however now also call record_failure when connection fails after connecting and before finishing mail upload, so we will change to another transport. I think it's fine that we will change the transport unnecessary. We want to change this logic later anyway, but the main reason to have less error types is that we will have less scenarios to test.
There was a problem hiding this comment.
We will also need to change SendResult later for #8640, to return the list of recipients and status for each one.
I drew the design that is implemented by this PR. There are many details missing in my drawing, e.g. MDNs are handled separately in the SMTP loop (and then als osent via the channels to the actors), and the actors do use the database a little bit ( |
Changing proxy settings requires restarting I/O anyway and at least Delta Chat for Android does it, so `Smtp` is recreated from scratch.
This is a preparation for running SMTP loop in the tests, to avoid having to create CancellationToken that is not going to be used.
It only contained arguments for smtp_loop(), now they are passed directly without the structure.
This change introduces `ConnectionActor` implementing the [actor pattern](https://ryhl.io/blog/actors-with-tokio/). The idea is to have a separate task for each transport that is responsible for maintaining an SMTP connection to the transport and can respond to two requests sent over the incoming channel: 1) connect to the transport and report back 2) send a message and report back. The tests can then run SMTP loop and reply to messages intended for `ConnectionActor` however they want and in any order, e.g. reporting success for the connection request or a "timeout", and then "SMTP temporary error" or "SMTP permanent error" or "network error" for the request to send a message. This way all the logic for recording successes and failures in the database, selecting which transports to send the message over etc. can be covered by offline tests, and timeouts can be emulated without actually waiting for a 1 minute timeout in the networking code.
4ff6562 to
c044c6f
Compare
c044c6f to
285a849
Compare
This PR is a refactoring to make SMTP failover code testable in Rust, as a follow-up to #8806 and preparation for more complicated failover strategies, e.g. sending the same messages over multiple transport.
Mostly repeating the commit message below:
Main change is the introduction of
Connectorimplementing the actor pattern. The idea is to have a separate task for each transport that is responsible for maintaining an SMTP connection to the transport and can respond to two requests sent over the incoming channel: 1) connect to the transport and report back 2) send a message and report back.The tests can then construct
crate::smtp::SmtpwithSmtpConnectorHandles (wrappers for the sending side of the actor message channel) manually constructed and reply to messages however they want, e.g. reporting success for the connection request or a "timeout", and then "SMTP temporary error" or "SMTP permanent error" or "network error" for the request to send a message.Networking code remains untested, but all the logic for recording successes and failures in the database, selecting which transports to send the message over etc. can be covered by offline tests, and timeouts can be emulated without actually waiting 1 minute for the timeout.