ContinuingHttpServletRequest: Make cookies immutable

[gissuebot]

From David.Ostrovsky on May 20, 2014 01:39:14

Description of the issue: Cookies are lost in Ccontinued background thread in ontinuingHttpServletRequest.

The whole purpose of request scope propagation is to make a
snapshot of the request.  To the snapshot belong all attributes
and cookies. Cookies are mutable and were erroneously not frozen.

Note this TODO comment in the corresponding code:

// TODO(dhanji): Cookies themselves are mutable. Is this a problem?

This breaks web session management in Gerrit Code review project.

Steps to reproduce: [1]

[1] https://gerrit-review.googlesource.com/57284

Attachment: gist
   0001-Guice-Servlet-Fix-request-propagation-problem.patch

Original issue: http://code.google.com/p/google-guice/issues/detail?id=806

[gissuebot]

From sberlin on May 20, 2014 06:09:30

I'll take a look.  FYI, if possible you should prefer using 'transferRequest' instead of 'continueRequest'.  It's usually better.

[gissuebot]

From David.Ostrovsky on May 20, 2014 06:38:31

Thanks. The provided patch is missing null pointer check in constructor:

  if (super.getCookies() != null) {
    cookies = super.getCookies().clone();
  }

In our use case the cookies exist only when ContinuingHttpServletRequest
object is created. Later in background thread no cookies available any more on the underlying wrapper HTTP request.

Cloning and freezing the cookies in constructor of ContinuingHttpServletRequest solves that.

[gissuebot]

From dhanji on May 20, 2014 10:02:39

Hi Sam I've got a better patch for this in the works if you can wait a day or two?

[gissuebot]

From dhanji on May 20, 2014 10:03:41

Hi this patch doesn't address the problem with isolating mutable cookies. I have a better patch I. The works, plz hold off for a little I'll have it in soon.

[gissuebot]

From sameb@google.com on May 20, 2014 10:05:06

Sure -- it'd be great to have you actively working on this again, Dhanji!

[gissuebot]

From dhanji on May 20, 2014 20:30:14

@all

Please review here: https://code.google.com/p/google-guice/source/detail?r=65f9d43bba9c447f01a3dcd1e2ebb5435b061324&name=continue_request_immutable_cookies This is on a branch.

Status: Accepted
Owner: dhanji

[gissuebot]

From dhanji on May 20, 2014 20:44:27

Actually, I lie, this is the correct commit: https://code.google.com/p/google-guice/source/detail?r=3b02622e856e845f9bc5353a494b5ccc75dec40c&name=continue_request_immutable_cookies Please disregard the earlier one.

[gissuebot]

From David.Ostrovsky on May 20, 2014 23:54:28

Thanks for the quick fix! I can confirm that this fix the original problem. Until a new version of Guice is release (new Beta?) i have uploaded a change to build Gerrit Code Review against Guice snapshot 4.0 [1].

[1] https://gerrit-review.googlesource.com/#/c/57306

[gissuebot]

From dhanji on May 21, 2014 00:12:45

Great, good to know. I'll merge as soon as it goes through code review.

Sam is your man for release questions.

Status: Started

[gissuebot]

From David.Ostrovsky on May 27, 2014 05:13:09

Any chance to release a new beta version of Guice with this patch included? We have to maintain our own fork of Guice for Gerrit Code Review to work on master [1]. Thanks.

[1] https://groups.google.com/forum/#!topic/repo-discuss/-ncxUgnxnrs

[gissuebot]

From David.Ostrovsky on June 12, 2014 23:00:12

Any ETA for new beta containing this fix?

[sameb]

Released in beta5.