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.

Sign up for GitHub

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

Jump to bottom

Logger init panic #57

Closed
wants to merge 8 commits into from
Closed

Logger init panic #57

wants to merge 8 commits into from

Conversation

YIDWang
Copy link
Contributor

@YIDWang YIDWang commented Jan 10, 2022

修复日志的 panic
Fixes #56

doujiang24
doujiang24 reviewed Feb 7, 2022
View reviewed changes
log/roller.go Outdated
@@ -41,6 +41,7 @@ var (
lumberjacksLocker sync.Mutex

errInvalidRollerParameter = errors.New("invalid roller parameter")
errTimeZero = errors.New("time is not zero")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errTimeZero = errors.New("time is not zero")
errTimeZero = errors.New("time should not be zero")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2022 •
edited

Codecov Report

Merging #57 (cb38cb3) into master (03081b9) will increase coverage by 0.51%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   44.71%   45.23%   +0.51%     
==========================================
  Files          45       46       +1     
  Lines        2630     2642      +12     
==========================================
+ Hits         1176     1195      +19     
+ Misses       1372     1368       -4     
+ Partials       82       79       -3
Impacted Files Coverage Δ
log/roller.go 91.74% <100.00%> (+0.15%) ⬆️
log/buffer.go 100.00% <0.00%> (ø)
buffer/iobuffer_pool.go 93.33% <0.00%> (+1.02%) ⬆️
log/logger.go 55.18% <0.00%> (+2.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03081b9...cb38cb3. Read the comment docs.

doujiang24
doujiang24 reviewed Feb 7, 2022
View reviewed changes
log/roller_test.go Outdated
if defaultRoller.MaxSize != 100 || defaultRoller.Compress != false {
t.Errorf("ParseRoller failed")
}
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'd better fix it instead of removing it. It's not a good idea to remove the existing test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

@doujiang24
Copy link
Member

@YIDWang Could you please paste the backtrace of panic? I'm not sure this PR is the proper way to fix it.

@YIDWang
Copy link
Contributor Author

YIDWang commented Feb 7, 2022

image

func TestGetOrCreate(t *testing.T) {
	roller := &Roller{
		MaxAge:     1,
		MaxSize:    10,
		MaxBackups: 3,
	}
	log, err := GetOrCreateLogger("/tmp/rollertest/test.log", roller)
	assert.NoError(t, err)

	log2, err := GetOrCreateLogger("/tmp/rollertest/test_bak.log", nil)
	assert.NoError(t, err)

	output := bytes.NewBuffer(nil)
	for output.Len() < 8*1024*1024 {
		output.WriteString(time.Now().String())
	}
	err = InitGlobalRoller("size=100 age=7 keep=10") //  Maxtime=10 change "size=100 age=7 keep=10"  panic
	assert.Error(t, err)

	for i := 0; i < 100; i++ {
		log.Write(output.Bytes())
		log2.Write(output.Bytes())
	}
	log.Close()
}

@doujiang24
Copy link
Member

@YIDWang Thanks, got it.
I think the proper way may be to reopen the logger.
otherwise, we have no way to change the MaxTime to zero which may break the existing behavior, it's allowed and useful before the logger started.

@YIDWang YIDWang closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doujiang24 doujiang24 doujiang24 left review comments

Assignees
No one assigned
Labels
cla:yes size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

日志库 Panic
3 participants
@YIDWang @codecov-commenter @doujiang24