time: fix TimeFormatter returning nil for the Unix epoch#5420
Open
Watson1978 wants to merge 1 commit into
Open
time: fix TimeFormatter returning nil for the Unix epoch#5420Watson1978 wants to merge 1 commit into
Watson1978 wants to merge 1 commit into
Conversation
TimeFormatter caches the last two formatted timestamps in two slots, and an
empty slot was marked by its key being 0. But 0 is a valid timestamp, so the
epoch matched the empty slot and the slot's nil string was returned instead
of a formatted time:
Fluent::TimeFormatter.new(nil, false, nil).format(0) #=> nil
Both cache paths are affected. format_without_subsec compares the key with
==, so only the epoch itself is hit. format_with_subsec compares it with
EventTime.eq?, which falls back to EventTime#== when either side is not an
EventTime, and that compares seconds alone -- so every timestamp within the
first second of the epoch is hit:
fmt = Fluent::TimeFormatter.new("%Y%m%d %H%M%S.%N", false, nil)
fmt.format(Fluent::EventTime.new(0, 123456789)) #=> nil
A slot stops carrying the key 0 once it is written, and the victim-selection
rule fills the second slot first, so the first slot keeps the key 0 until two
distinct timestamps have been formatted. The failure is therefore confined to
a cold cache, with one exception: a stream whose timestamps are all the epoch
returns early on every call, never fills a slot, and loses the time column on
every line for as long as the process runs.
Mark an empty slot by its cached string being nil instead. format_nocache
always returns a String, so a filled slot can never be mistaken for an empty
one, whatever its key.
Signed-off-by: Shizuo Fujita <fujita@clear-code.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which issue(s) this PR fixes:
Fixes #
What this PR does / why we need it:
TimeFormattercaches the last two formatted timestamps in two slots, and an empty slot was marked by its key being0. But0is a valid timestamp, so the epoch matched an empty slot and itsnilstring was returned as a cache hit:format_with_subseccompares slots withEventTime.eq?, which falls back toEventTime#==and compares seconds alone, so there the whole first second of the epoch is affected:The fix is to mark an empty slot by its cached string being
nilrather than by its key.format_nocachealways returns aString, so a filled slot can never be mistaken for an empty one. Every other timestamp behaves exactly as before, since key0could never have matched it.Impact is small, and I don't think this is urgent.
A slot keeps the key
0only until it is written, so a formatter self-heals once it has formatted two distinct timestamps, within a second of the first record.The exception is a stream whose timestamps are all the epoch: the early return never fills a slot, and
out_filethen writes every line with an empty time column.That is reachable via
time_type unixtime, which parses withvalue.to_iand turns an empty or non-numerictime_keyintoEventTime.new(0), but such a pipeline is misconfigured anyway. The reason to fix it is simply thatnilis never a correct result.Docs Changes:
Release Note: