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.

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

Fix an issue caused by unexpected stop of trace sessions when TraceEventSession is just created and attached to check if a session is active. #929

Merged
merged 3 commits into from
May 9, 2019

Conversation

ZhaminXu
Copy link
Contributor

@ZhaminXu ZhaminXu commented May 5, 2019

Fix an issue caused by unexpected stop of trace sessions when TraceEventSession is just created and attached to check if a session is active.

…entSession is just created and attached to check if a session is active.
@codecov-io
Copy link

codecov-io commented May 5, 2019

Codecov Report

Merging #929 into master will increase coverage by 0.13%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
+ Coverage   17.66%    17.8%   +0.13%     
==========================================
  Files         228      230       +2     
  Lines      139621   139889     +268     
  Branches    12379    12406      +27     
==========================================
+ Hits        24667    24903     +236     
- Misses     114008   114035      +27     
- Partials      946      951       +5
Flag Coverage Δ
#2017 17.8% <0%> (+0.13%) ⬆️
#Debug 17.8% <0%> (+0.13%) ⬆️
Impacted Files Coverage Δ
src/TraceEvent/TraceEventSession.cs 2.22% <0%> (-0.01%) ⬇️
src/TraceEvent/BPerf/BPerfEventSource.cs 86.59% <0%> (ø)
src/TraceEvent/TraceEvent.Tests/BPerfTest.cs 83.13% <0%> (ø)
src/TraceEvent/TraceEvent.cs 63.71% <0%> (+0.14%) ⬆️
src/TraceEvent/DynamicTraceEventParser.cs 66.81% <0%> (+0.19%) ⬆️
src/TraceEvent/RegisteredTraceEventParser.cs 46.93% <0%> (+0.48%) ⬆️

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 f76a800...3425ac2. Read the comment docs.

@ZhaminXu
Copy link
Contributor Author

ZhaminXu commented May 5, 2019

We encountered an issue when trying to use latest TraceEvent nuget package. Trace sessions are stopped unexpected due to GC-ed TraceEventSession objects. So propose this fix, as current logic will caused trouble to users whoever uses GetActionSession method right after creating the real trace session.

@brianrob
Copy link
Member

brianrob commented May 6, 2019

@ZhaminXu, thanks for the contribution. Do you know which line(s) in Dispose() are causing the attached session to be closed? I'm concerned that we might leak resources if Dispose doesn't get called at all even in the attach case.

@ZhaminXu
Copy link
Contributor Author

ZhaminXu commented May 7, 2019

@ZhaminXu, thanks for the contribution. Do you know which line(s) in Dispose() are causing the attached session to be closed? I'm concerned that we might leak resources if Dispose doesn't get called at all even in the attach case.
Yes, it's Stop(true) method. I also checked more code there. If TraceEventSession is created and attached, m_sessionHandle is invliad, m_source and m_kernelSession are not used. So I think it's safe to skip the entire dispose method. But after thinking carefully, I think your concern is valid. let me try to just skip the stop method.

Copy link
Member

@brianrob brianrob left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@brianrob brianrob merged commit 55622b9 into microsoft:master May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants