Fix race condition causing interleaved logs #11432
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JosiahWI
force-pushed
the
fix/dump-header-race
branch
from
a4011d0 to
b4335e5
Compare
Contributor
Author
|
LSan detected a memory leak in the ImageMagick plugin. |
bneradt
previously requested changes
Jun 8, 2024
Contributor
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Looks good. It's a big bonus that this makes things more efficient when debug logging is on.
Contributor
Author
|
The regression test PARENT_SELECTION failed. |
bneradt
reviewed
Jun 8, 2024
Contributor
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Looks good to me. I'll let a non-Yahoo reviewer chip in.
bneradt
dismissed
their stale review
Releasing my blocking review because all changes are done.
zwoop
reviewed
Jun 9, 2024
cmcfarlen
previously approved these changes
Jun 10, 2024
This removes the unused heap argument and updates calls across the whole codebase accordingly.
This constifies some arguments to mime_field_print and mime_hdr_print and updates usage throughout the codebase accordingly.
This removes the heap argument from the method and updates the usage throughout the codebase.
This marks the hdr argument to http_hdr_print as const.
The output of the `DUMP_HEADER` macro was being written in multiple calls to
fprintf() that raced with other logging, resulting in interleaved output that
intermittently failed the ja3_fingerprint AuTest on CI. Whether other AuTest
failures were related to the same issue is unknown at this point.
This fixes the race condition by batching the calls to a single fprintf() call.
This also refactors the macro to two inline functions. In a benchmark using an
expanded version of the headers in the HttpTransact tests, a 60% speedup was
measured over the macro if all the header data fit in the buffer. In the worst
case that the buffer could only hold a byte at a time, the speedup was 80%.
The benchmark was compiled with -O3 and stderr was piped to /dev/null.
These were the headers used for the benchmark:
```
header input1[] = {
{"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", "111111111111111111111111"},
{"BBB", "222" },
{"CCC", "333" },
{"DDD", "333" },
{"EEE", "333" },
{"FFF", "333" },
{"GGG", "333" },
{"HHH", "333" },
{"III", "333" },
{"JJJ", "333" },
{"KKK", "333" },
};
```
This fixes apache#11431.
This makes the following changes: * The length parameter to string::append is used instead of adding a null terminator. * Some erroneous logic left over from testing is fixed.
This is an API change. The parameter is no longer used, so this commit removes it and updates the developer documentation and core plugins accordingly.
JosiahWI
force-pushed
the
fix/dump-header-race
branch
from
7661666 to
4b8cfe5
Compare
Contributor
Author
|
Rebased and did the changes requested by @zwoop. |
Since the API changed, the example plugins now need updating as well.
Contributor
Author
|
The AuTest bad_chunked_encoding failed with a very interesting message: |
zwoop
approved these changes
Jun 11, 2024
Contributor
|
Cherry-picked to v10.0.x |
cmcfarlen
pushed a commit
that referenced
this pull request
Jun 12, 2024
* Remove unused argument from mime_hdr_print
This removes the unused heap argument and updates calls across the whole
codebase accordingly.
* Constify some arguments for MIME printing
This constifies some arguments to mime_field_print and mime_hdr_print and
updates usage throughout the codebase accordingly.
* Remove unused argument from http_hdr_print
This removes the heap argument from the method and updates the usage
throughout the codebase.
* Constify argument to http_hdr_status_get
* Constify argument to http_hdr_print
This marks the hdr argument to http_hdr_print as const.
* Constify the HTTPHdr::print method
* Fix race condition causing interleaved logs
The output of the `DUMP_HEADER` macro was being written in multiple calls to
fprintf() that raced with other logging, resulting in interleaved output that
intermittently failed the ja3_fingerprint AuTest on CI. Whether other AuTest
failures were related to the same issue is unknown at this point.
This fixes the race condition by batching the calls to a single fprintf() call.
This also refactors the macro to two inline functions. In a benchmark using an
expanded version of the headers in the HttpTransact tests, a 60% speedup was
measured over the macro if all the header data fit in the buffer. In the worst
case that the buffer could only hold a byte at a time, the speedup was 80%.
The benchmark was compiled with -O3 and stderr was piped to /dev/null.
These were the headers used for the benchmark:
```
header input1[] = {
{"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", "111111111111111111111111"},
{"BBB", "222" },
{"CCC", "333" },
{"DDD", "333" },
{"EEE", "333" },
{"FFF", "333" },
{"GGG", "333" },
{"HHH", "333" },
{"III", "333" },
{"JJJ", "333" },
{"KKK", "333" },
};
```
This fixes #11431.
* Implement changes requested by Brian Neradt
This makes the following changes:
* The length parameter to string::append is used instead of adding a
null terminator.
* Some erroneous logic left over from testing is fixed.
* Fix formatting
* Add log output to dump_header for invalid hdrs
* Remove bufp parameter from TSMimeHdrPrint
This is an API change. The parameter is no longer used, so this commit removes
it and updates the developer documentation and core plugins accordingly.
* Fix the example plugins
Since the API changed, the example plugins now need updating as well.
(cherry picked from commit 054b6a8)
maskit
added a commit
to maskit/trafficserver
that referenced
this pull request
Aug 15, 2024
This is for apache#8066, apache#8804, apache#9509 and apache#11432.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The output of the
DUMP_HEADERmacro was being written in multiple calls tofprintf() that raced with other logging, resulting in interleaved output that
intermittently failed the ja3_fingerprint AuTest on CI. Whether other AuTest
failures were related to the same issue is unknown at this point.
This fixes the race condition by batching the calls to a single fprintf() call.
This also refactors the macro to two inline functions. In a benchmark using an
expanded version of the headers in the HttpTransact tests, a 60% speedup was
measured over the macro if all the header data fit in the buffer. In the worst
case that the buffer could only hold a byte at a time, the speedup was 80%.
The benchmark was compiled with -O3 and stderr was piped to /dev/null.
These were the headers used for the benchmark:
This fixes #11431.