grails / grails-mail

The Grails Mail Plugin
https://grails.github.io/grails-mail/
Apache License 2.0
14 stars 25 forks source link

Application-wide rendering side effects caused by incorrect usage of WrappedResponseHolder #23

Closed davidkron closed 5 years ago

davidkron commented 7 years ago

I recently had to do some major debugging of a hard to reproduce issue. The issue occurred mainly when rendering grails templates in the controller and retrieving them by AJAX. Sometimes (it seemed pretty random) the response to exactly the same request didn't contain any Content-Type header, which resulted in messed up pages (mainly because of missing charset).

I finally managed to reproduce the issue and discovered that the source of the problem was caused by the mail plugin's HTML rendering.

Detailed description

Correct usage of WrappedResponseHolder

I searched the usage of WrappedResponseHolder inside of grails. The correct pattern to use this class seems to be like this:

  1. retrieve current wrapped response
  2. set a new wrapped response
  3. do something
  4. restore wrapped response to previous value
HttpServletResponse previousWrappedResponse = WrappedResponseHolder.getWrappedResponse();
try {
   WrappedResponseHolder.setWrappedResponse(wrappedResponse);
   ...
}
finally {
    WrappedResponseHolder.setWrappedResponse(previousWrappedResponse);
}

Wrong usage in mail plugin

The code is found in the class MailMessageContentRenderer.RenderEnvironment: WrappedResponseHolder.wrappedResponse = originalRequestAttributes?.currentResponse

This line of code sets the wrapped response after the rendering. But is uses the current response instead of the previously stored value before doing the rendering.

Example project

I made an example project to reproduce and demonstrate the issue: https://github.com/davidkron/mail-plugin-renderissue

Details of the project:

To see the issue, run the application, call the following URLs and see the DEBUG messages in the console:

demon101 commented 5 years ago

@jeffbrown , as I can see the issue can be closed. PR already part of 2.0.0 version

davidkron commented 5 years ago

I will close this issue. In our applications we could verify that the fix is working successfully.