kaisellgren / mailer

Compose and send emails from Dart. Supports file attachments, HTML emails and multiple transport methods.
MIT License
166 stars 86 forks source link

Very HARD BUG in your code! #252

Closed darkstarx closed 2 months ago

darkstarx commented 2 months ago

Hi! You should close the connection on failure.

The method _send is unsafe:

Future<SendReport> _send(
    Message message, Connection connection, Duration? timeout) async {
  var messageSendStart = DateTime.now();
  DateTime messageSendEnd;
  try {
    await client.sendSingleMessage(message, connection, timeout);
    messageSendEnd = DateTime.now();
  } catch (e) {
    _logger.warning('Could not send mail.', e);
    rethrow;          // <<< This method is UNSAFE
  }
  // If sending the message was successful we had to open a connection and
  // `connection.connectionOpenStart` can no longer be null.
  return SendReport(message, connection.connectionOpenStart!, messageSendStart,
      messageSendEnd);
}

But you don't catch exceptions in the method send:

Future<SendReport> send(Message message, SmtpServer smtpServer,
    {Duration? timeout}) async {
  _validate(message);
  var connection = await client.connect(smtpServer, timeout);
  var sendReport = await _send(message, connection, timeout);
  await client.close(connection);  // <<< Forget to CLOSE the connection!
  return sendReport;
}

Use try-finally here:

  _validate(message);
  final connection = await client.connect(smtpServer, timeout);
  try {
    final sendReport = await _send(message, connection, timeout);
    return sendReport;
  } finally {
    await client.close(connection);  // <<< Forget to CLOSE the connection!
  }

And please use final instead of var when you aren't going to change the value.

close2 commented 2 months ago

@darkstarx Thanks for reporting and providing a fix.

Your bug report has a very aggressive tone. As I hope that this wasn't your intention, I decided to mention it. My initial reaction wasn't to be thankful for your provided fix, but to be angry at your accusation ("Very HARD BUG").

It is also considered rude to tell somebody to do something. "And please use final...".

Again, I don't think that this was your intention, but I know other programmers which would try to be as unhelpful as possible with such a bug report.

darkstarx commented 2 months ago

Hi @close2! This is a cultural aspect that does not concern the subject of an issue. I just gave some good recommendations to the author of the package, as shortly as i had time for it. The less bugs, the higher rating of the package and better reputation of the author. Thanks.