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

*: fix setHeapProfileTracker panic on Windows #17226

Merged
merged 4 commits into from
May 18, 2020
Merged

*: fix setHeapProfileTracker panic on Windows #17226

merged 4 commits into from
May 18, 2020

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented May 14, 2020 •
edited
Loading

Signed-off-by: Shuaipeng Yu jackysp@gmail.com

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:
In #16777, util/profile/trackerRecorder.go uses logrus as its logger, but it panicked on Windows. I'm not sure if it is a bug of logrus or not.

runtime: bad pointer in frame github.com/konsorten/go-windows-terminal-sequences.EnableVirtualTerminalProcessing at 0xc0009cf680: 0x380
fatal error: invalid pointer found on stack
goroutine 239 [copystack]:
runtime.concatstring2(0xc0009cf3b0, 0x23cd9ea, 0xc, 0x23bca62, 0x1, 0x0, 0x0)
        c:/go/src/runtime/string.go:57 +0x71 fp=0xc0009cf378 sp=0xc0009cf370 pc=0x451c21
syscall.UTF16FromString(0x23cd9ea, 0xc, 0x0, 0x0, 0x0, 0x0, 0x0)
        c:/go/src/syscall/syscall_windows.go:45 +0xcc fp=0xc0009cf460 sp=0xc0009cf378 pc=0x47fb7c
syscall.UTF16PtrFromString(...)
        c:/go/src/syscall/syscall_windows.go:91
syscall.LoadDLL(0x23cd9ea, 0xc, 0xc0009b3558, 0x442473, 0xc000077680)
        c:/go/src/syscall/dll_windows.go:69 +0x54 fp=0xc0009cf518 sp=0xc0009cf460 pc=0x47a464
syscall.(*LazyDLL).Load(0xc000109280, 0x0, 0x0)
        c:/go/src/syscall/dll_windows.go:236 +0xc7 fp=0xc0009cf568 sp=0xc0009cf518 pc=0x47c277
syscall.(*LazyProc).Find(0xc000112690, 0x0, 0x0)
        c:/go/src/syscall/dll_windows.go:291 +0xbe fp=0xc0009cf5c0 sp=0xc0009cf568 pc=0x47c56e
syscall.(*LazyProc).mustFind(0xc000112690)
        c:/go/src/syscall/dll_windows.go:309 +0x32 fp=0xc0009cf5e8 sp=0xc0009cf5c0 pc=0x47c692
syscall.(*LazyProc).Call(0xc000112690, 0xc00004c300, 0x2, 0x2, 0x24ae4b0, 0xc0009b36b8, 0x485ea6, 0x7ffb93a82820)
        c:/go/src/syscall/dll_windows.go:327 +0x36 fp=0xc0009cf638 sp=0xc0009cf5e8 pc=0x47c766
github.com/konsorten/go-windows-terminal-sequences.EnableVirtualTerminalProcessing(0x380, 0x1, 0x0, 0xc0009b36ec)
        C:/Users/yushu/go/pkg/mod/github.com/konsorten/go-windows-terminal-sequences@v1.0.2/sequences.go:30 +0xcb fp=0xc0009cf698 sp=0xc0009cf638 pc=0xb27bcb
github.com/sirupsen/logrus.initTerminal(0x28ace20, 0xc000006020)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/terminal_check_windows.go:16 +0x64 fp=0xc0009cf6c8 sp=0xc0009cf698 pc=0xb2da94
github.com/sirupsen/logrus.checkIfTerminal(0x28ace20, 0xc000006020, 0x70e58)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/terminal_check_windows.go:31 +0xb4 fp=0xc0009cf700 sp=0xc0009cf6c8 pc=0xb2db74
github.com/sirupsen/logrus.(*TextFormatter).init(...)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/text_formatter.go:86
github.com/sirupsen/logrus.(*TextFormatter).Format.func1()
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/text_formatter.go:170 +0x58 fp=0xc0009cf730 sp=0xc0009cf700 pc=0xb31368
sync.(*Once).doSlow(0xc000123210, 0xc0009b3960)
        c:/go/src/sync/once.go:66 +0xf3 fp=0xc0009cf780 sp=0xc0009cf730 pc=0x4782a3
sync.(*Once).Do(...)
        c:/go/src/sync/once.go:57
github.com/sirupsen/logrus.(*TextFormatter).Format(0xc0001231d0, 0xc0003183f0, 0xc0009b3d38, 0xb31204, 0x2194640, 0xc0009ace40, 0xc0009b3d80)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/text_formatter.go:170 +0xf1d fp=0xc0009cfd08 sp=0xc0009cf780 pc=0xb2ec4d
github.com/sirupsen/logrus.(*Entry).write(0xc0003183f0)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/entry.go:255 +0x83 fp=0xc0009cfd90 sp=0xc0009cfd08 pc=0xb29433
github.com/sirupsen/logrus.Entry.log(0xc00010cc40, 0xc0009ace10, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/entry.go:231 +0x1a5 fp=0xc0009cfe08 sp=0xc0009cfd90 pc=0xb290d5
github.com/sirupsen/logrus.(*Entry).Log(0xc00021ff80, 0xc000000004, 0xc0009b3fa8, 0x1, 0x1)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/entry.go:268 +0xf2 fp=0xc0009cff10 sp=0xc0009cfe08 pc=0xb296c2
github.com/sirupsen/logrus.(*Logger).Log(0xc00010cc40, 0xc000000004, 0xc0009b3fa8, 0x1, 0x1)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/logger.go:192 +0x84 fp=0xc0009cff50 sp=0xc0009cff10 pc=0xb2c434
github.com/sirupsen/logrus.(*Logger).Info(...)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/logger.go:206
github.com/sirupsen/logrus.Info(...)
        C:/Users/yushu/go/pkg/mod/github.com/sirupsen/logrus@v1.4.2/exported.go:109
github.com/pingcap/tidb/util/profile.HeapProfileForGlobalMemTracker(0xdf8475800)
        C:/Users/yushu/work/tidb/util/profile/trackerRecorder.go:27 +0x89 fp=0xc0009cffd8 sp=0xc0009cff50 pc=0x1484439
runtime.goexit()
        c:/go/src/runtime/asm_amd64.s:1373 +0x1 fp=0xc0009cffe0 sp=0xc0009cffd8 pc=0x469551
created by main.setHeapProfileTracker
        C:/Users/yushu/work/tidb/tidb-server/main.go:282 +0x94

What is changed and how it works?

Proposal: xxx

What's Changed:
Actually logrus is only used by slow query due to some historical issue. TiDB uses pingcap/log (zap) as its default logger. So I change it and set the tracker after the logger initialized.

How it Works:

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    go run tidb-server/main.go on Windows, and it won't panic.

Release note

  • No release note, because the Windows support is only for debugging.

@jackysp
*: fix setHeapProfileTracker panic on Windows
c47918b 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp jackysp requested review from Yisaer and SunRunAway May 14, 2020 13:50
@Yisaer
Copy link
Contributor

Yisaer commented May 14, 2020

LGTM

@jackysp jackysp added component/server type/bugfix This PR fixes a bug. status/LGT1 Indicates that a PR has LGTM 1. labels May 14, 2020
@jackysp
Copy link
Member Author

jackysp commented May 15, 2020

PTAL @SunRunAway

@Yisaer
Copy link
Contributor

Yisaer commented May 15, 2020

@eurekaka @lzmhhh123 PTAL

lzmhhh123
lzmhhh123 approved these changes May 18, 2020
View reviewed changes
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 18, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

@jackysp merge failed.

@codecov
Copy link

codecov bot commented May 18, 2020 •
edited
Loading

Codecov Report

Merging #17226 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #17226   +/-   ##
===========================================
  Coverage   79.9588%   79.9588%           
===========================================
  Files           517        517           
  Lines        140361     140361           
===========================================
  Hits         112231     112231           
  Misses        19177      19177           
  Partials       8953       8953

@jackysp
Copy link
Member Author

jackysp commented May 18, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

@jackysp merge failed.

@jackysp
Copy link
Member Author

jackysp commented May 18, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

@jackysp merge failed.

crazycs520
crazycs520 reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member Author

jackysp commented May 18, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

@jackysp merge failed.

@jackysp
Copy link
Member Author

jackysp commented May 18, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

/run-all-tests

@sre-bot sre-bot merged commit 4a67c3a into pingcap:master May 18, 2020
@jackysp jackysp deleted the tracker_panic branch November 25, 2020 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@lzmhhh123 lzmhhh123 lzmhhh123 approved these changes

@Yisaer Yisaer Awaiting requested review from Yisaer

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees
No one assigned
Labels
component/server status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jackysp @Yisaer @sre-bot @crazycs520 @lzmhhh123