Skip to content
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

Make AccessEvent.prepareForDeferredProcessing() to create a copy of the ... #240

Merged
merged 2 commits into from
Jan 28, 2015

Conversation

dimas
Copy link

@dimas dimas commented Jan 24, 2015

...request attributes map so attributes are available later even if processing is done in a background thread.

Only Serializable attributes are copied to be on a safe side. Also, do not copy attributes set by Logback's TeeFilter as the very same information is already serialised as request/response content.

@tony19. this PR closes LOGBACK-1033 and superceedes the PR #238. How it is different:

  • implementation changed to be more on the safe side and use the old logic (of getting request attribute directly from httpRequest) as long as httpRequest is available. Only if event was actually serialised and deserialised and transient httpRequest is lost, the copy of the attribute map will be used
  • it only copied objects that are Serializable - again, to be on a safe side in case event is not just passed to a background thread but serialised into file or something
  • unit test is added

Release notes:

AccessEvent now creates a copy of request attributes when its prepareForDeferredProcessing() method is called. This makes attributes visible even if appender uses a background thread to process events. (LOGBACK-1033).

…he request attributes map so attributes are available later even if processing is done in a background thread.

Only Serializable attributes are copied to be on a safe side. Also, do not copy attributes set by Logback's TeeFilter as the very same information is already serialised as request/response content.
@dimas
Copy link
Author

dimas commented Jan 24, 2015

Re the build failure - I do not understand that error at all and all the 4 files I modified are in logback-access. None of them is in -classic that has failed. So I do not think it is me. More like I created my fork off already broken build.
(btw, mvn clean install passes successfully on my laptop so I am not sure what Travis CI is unhappy about)

…le because web containers may recycle it and populate with the fields from another request. So make sure that when prepareForDeferredProcessing() is called, we will be only using copied data.
@dimas
Copy link
Author

dimas commented Jan 25, 2015

Please ignore my words that this implementation uses old logic and gets attributes from the original request as long as it is available. Apparently AccessEvent does not reset httpRequest when deferred processing is requested even thought it should never consult it again because web container may recycle that request and by the moment getAttribute() is called, iit will be something else.
I fixed it and also added a test that attributes are captured at the moment deferred processing is requested.

You may also consider a bigger fix - to null httpRequest+httpResponse in AccessEvent when prepareForDeferredProcessing() is called. I did not do it myself because it is obviously more riski - I do not know if it will affect anyone. But it is a right thing - we should never access request/response which may have been recycled.

tony19 added a commit that referenced this pull request Jan 28, 2015
Make AccessEvent.prepareForDeferredProcessing() to create a copy of the ...
@tony19 tony19 merged commit 5909fcb into qos-ch:master Jan 28, 2015
@tony19
Copy link
Contributor

tony19 commented Jan 28, 2015

Nice work. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants