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

Upgrade to mailcomposer@4 and smtp-connection@2 #8495

Merged
merged 1 commit into from Mar 22, 2017

Conversation

Projects
None yet
5 participants
@edemaine
Contributor

edemaine commented Mar 17, 2017

I wrote this patch to fix #8425, namely, to fix how long header lines get encoded. Main discussion is there.

  • Switch from mailcomposer 0.1.15 -> 4.0.1 (latest, still MIT license).
  • Switch from simplesmtp (which only supports mailcomposer 0.1) to smtp-connection (which supports any mail composer).
  • Use smtp-connection@2 (instead of latest) which shares nodemailer-shared codebase with mailcomposer 4.0.1.
  • Add test for long header lines (the original bug being fixed here)
  • Add extra test for HTML + text messages
  • Document some extra options arguments supported by new mailcomposer. (I didn't add them all -- let me know if I should, or link to that in some way...)
Upgrade to mailcomposer@4 and smtp-connection@2. Fix #8425
* Switch from mailcomposer 0.1.15 -> 4.0.1 (latest, still MIT license).
* Switch from simplesmtp (which only supports mailcomposer 0.1)
  to smtp-connection (which supports any mail composer).
* Use smtp-connection@2 (instead of latest) which shares
  nodemailer-shared codebase with mailcomposer 4.0.1.
* Add test for long header lines (the original bug being fixed here)
* Add extra test for HTML + text messages
* Document some extra options arguments supported by new mailcomposer

@edemaine edemaine force-pushed the edemaine:devel branch from 6756c7b to 7b6d629 Mar 17, 2017

@abernix

I'll give this another look over tomorrow, but preliminary scan looks great. Thanks very much for tackling this!

One request though... CircleCI has decided not to run tests for this at all since (I'm guessing) you have a CircleCI account linked to your GitHub account and meteor/meteor is not enabled to run tests. (I understand why they prefer your CircleCI account if you have one and it's configured for your fork, but considering they use meteors if you don't have an account, it's confusing that they don't use meteors if you don't have it configured. *sigh*).

Do you mind enabling CircleCI on your account for meteor? I think if you rebuild the same branch, it will update this PR with the status.

"\r\n" +
"body\r\n" +
"I =E2=99=A5 Meteor" +

This comment has been minimized.

@abernix
@edemaine

This comment has been minimized.

Contributor

edemaine commented Mar 22, 2017

Enabled, and tests executed. (No, I didn't have a CircleCI account before, I don't think. At least it asked for GitHub authentication for the first time, and sent me a welcome message after joining...)

@benjamn benjamn merged commit fe380e3 into meteor:devel Mar 22, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjamn benjamn added this to the Release 1.4.4 milestone Mar 22, 2017

@abernix

This comment has been minimized.

Member

abernix commented Mar 22, 2017

Thanks so much for working on this, @edemaine. We've got this slated for 1.4.4 right now. :)

@edemaine

This comment has been minimized.

Contributor

edemaine commented Mar 23, 2017

Awesome, thanks! I look forward to putting it in my Meteor app.

abernix added a commit that referenced this pull request Mar 28, 2017

Upgrade to mailcomposer@4 and smtp-connection@2. Fix #8425 (#8495)
* Switch from mailcomposer 0.1.15 -> 4.0.1 (latest, still MIT license).
* Switch from simplesmtp (which only supports mailcomposer 0.1)
  to smtp-connection (which supports any mail composer).
* Use smtp-connection@2 (instead of latest) which shares
  nodemailer-shared codebase with mailcomposer 4.0.1.
* Add test for long header lines (the original bug being fixed here)
* Add extra test for HTML + text messages
* Document some extra options arguments supported by new mailcomposer
@abernix

This comment has been minimized.

Member

abernix commented Mar 30, 2017

This should be available for testing in Meteor 1.4.4-rc.4. Please try it out and provide feedback! 😄

meteor update --release 1.4.4-rc.4
@jbbr

This comment has been minimized.

jbbr commented Apr 10, 2017

Unfortunately this caused me a lot of trouble as I got some random, endless loading situations because meteor methods containing Email.send calls never finished execution.

Email.send is a synchronous fiber call but in approximately 30% of the calls it never finishes nor throws an exception after the upgrade.

I'll try to reproduce this in a clean example repo.

For anyone else who might experience this issue: Downgrading & pinning email & accounts-password fixes the problem for now-

meteor add email@=1.1.18 accounts-password@=1.3.4

(only works in 1.4.3.x - 1.4.4 requires email >=1.2.0)

Anyway thanks for this PR.

@abernix

This comment has been minimized.

Member

abernix commented Apr 10, 2017

@jbbr Thanks for reporting. Please do try to make a reproduction and report it in a new issue so it can be looked into!

@edemaine

This comment has been minimized.

Contributor

edemaine commented Apr 10, 2017

Ouch, sorry to hear this. That's definitely unacceptable. I'll try to reproduce as well...

@harrisonhunter

This comment has been minimized.

harrisonhunter commented Apr 10, 2017

I am seeing similar behavior when using 1.4.4.1 (albeit less frequently) that results in this error:
Error: Unexpected Response: 421 Timeout. Try talking faster next time! and failure to send

and it is resolved by downgrading.

@edemaine

This comment has been minimized.

Contributor

edemaine commented Apr 10, 2017

Thanks for the details. My guess is that the new smtp-connection package is not auto-retrying the connection when it sits idle and disconnects, whereas (I'm guessing) the old package did so. This will require some reworking...

@abernix

This comment has been minimized.

Member

abernix commented Apr 12, 2017

@jbbr any updates on trying to create a reproduction? It sounds like it might also be important to include information on who your SMTP provider is in order to properly reproduce this.

@jbbr

This comment has been minimized.

jbbr commented Apr 13, 2017

@abernix sorry for the delay.
I just tried reproducing the problem in a clean meteor project. Right now everything seems to work fine.
SMTP providers are Mailtrap (dev) and Mailgun (production).

I experienced the problem with both and I assume it's not due to network or SMTP errors like @harrisonhunter is suggesting. Such errors usually trigger an exception and can be catched or are at least dumped into the console. In my case nothing happened at all. The Meteor-method call just kind of paused without a sign or error or timeout.

As I'm using async/await syntax and promises I might also have forgot a "catch" somewhere and missed the error (Errors in promise callbacks/async functions are silently ignored...). So I'll try again with the problematic project and open a new issue if I'm able to reproduce this.

EDIT: I was able to kind of reproduce this in a clean project. It does not seem to require anything special - Just try many times to send a mail within a meteor-method with SMTP configured (Mailgun in my case).
See #8596 for details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment