Fix Audit Message Logging Interceptor Race Condition - #938
Conversation
|
/test test-end-to-end-redis-cluster |
|
/test test-end-to-end-batch |
|
/test test-end-to-end-redis-cluster |
|
/test test-end-to-end-batch-dataflow |
|
/test test-end-to-end |
|
/test test-end-to-end-redis-cluster |
There was a problem hiding this comment.
Should this super.sendMessage(message); not be the last call that is made?
There was a problem hiding this comment.
This was done to ensure that logging code, as much as possible stays out of the way of the rest of message (ie exceptions in logging code do not disrupt message handling.) Going to try as last call to see if it fixes the concurrency issue.
There was a problem hiding this comment.
From my testing, moving the location of statements does not resolve the issue.
|
/test test-end-to-end-batch-dataflow |
…e if message cannot be recorded.
…to be called from multiple async calls
…not found by log4j2
…duced under load.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrzzy, woop The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/test test-end-to-end-redis-cluster |
Why we need it:
Under load the
GrpcMessageInterceptorwill throwIllegalStateExceptiondue to a race condition:GrpcMessageInterceptorexpects thatonMessage(),onHalfClose(),close()are called in that order.close()is being called beforeonMessage(), causing theMessageAuditLogEntryto be missingrequest, causingIllegalStateExceptionwhen.build()is called.What this PR does:
Fixes
GrpcMessageInterceptor's race condition by allowingclosed()to be called beforeonMessage()onMessage().Adds an integration test
CoreLoggingITto verify that Message Audit Logs are produced correctly.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: