Teammates: SendgridService: fix successful request detection

Created on 17 Mar 2018  路  7Comments  路  Source: TEAMMATES/teammates

v6.4.0

This version started creating lot of teammates.common.util.Logger severe: Email failed to send: (Logger.java:52) although emails seems to be going through.

I've rolled back to v6.3.0 for now.

a-FaultTolerance p.High

Most helpful comment

How about if (response.getStatusCode() < 200 || response.getStatusCode() > 299)? Since all 2xx codes are considered success.

All 7 comments

@whipermr5 Could this be a false positive because we increased error logging for task queues recently?

Unlikely as the increased logging was in 6.3.0 too. Related to #8587? Sendgrid was updated.

@damithc I've tested the Sendgrid new API on my staging, but I won't rule out that happening yet. Can try if switching to some other email service work?
Also, does the error logs in GAE show any further information? Does the Sendgrid logs show that the emails are actually sent?

Yes, the email is sent. I checked sendgrid activity log for a few cases and even tried sending an email to myself (I received it, but our log stated it as 'failed')

Confirmed: Tried sending reminder to myself using latest version and previous version. Received all emails but the current version logged errors for all 3 emails:
image
There are no other info in the log other than teammates.common.util.Logger severe: Email failed to send: (Logger.java:52). We can try deploying a hotfix that will log more info.

Found the cause. The email sending code treats status code other than 200 OK as anomaly, but Sendgrid v3 API (indeed, upgraded in V6.4.0) returns 202 ACCEPTED as successful status code. (reference)

Simply change this line:
https://github.com/TEAMMATES/teammates/blob/8a974ee703c46f4b3280552f46b3e767eb561eab/src/main/java/teammates/logic/core/SendgridService.java#L65

To:

- if (response.getStatusCode() != SUCCESS_CODE) {
+ if (response.getStatusCode() != SUCCESS_CODE && response.getStatusCode() != 202) {

How about if (response.getStatusCode() < 200 || response.getStatusCode() > 299)? Since all 2xx codes are considered success.

Was this page helpful?
0 / 5 - 0 ratings