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.
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
add hooks to debug OpenSSL memory #101626
add hooks to debug OpenSSL memory #101626
Changes from 23 commits
a163dbf
9b5b3d3
ea6ac3f
0338b06
8744c2d
401178f
803fc94
cd55d63
638df97
dd9b017
d42f271
01f556f
173827a
a86c713
c0838ea
60e933e
7c03ee3
1fe580f
c0e1a98
2fc9779
9c3da4f
ac41d7f
7f0885b
67d70ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original PR had
IntPtr
, is there some reason to useUIntPtr
overIntPtr
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was browsing PRs and accidentally stumbled upon this one.
Is there a specific reason this code uses obsolete
Tuple
overValueTple
aka(UIntPtr, int, string)
? Thanks!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the
_allocations.Count
value change while iterating over_allocations
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. I expect the
ConcurrentDictionary
to handle parallel access gracefully but you may not get consistent snapshot. We did it while back with theHashSet
and manual locking. Since the iteration can take a while I feel it is better to allow concurrent access and deliver what we can. In ideal situation one would enable tracking run the repro and dump whatever it remains.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that the new size may not necessarily match the size of the
allocations
array, andallocations[index]
access may throw.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also,
ConcurrentDictionary.Count
is extremely slow.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the field is not used anywhere, can we move the call to
GetMemoryDebug
to cctor?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be moved to Interop.Crypto, because following code
Will enable tracking, but later when Interop.OpenSsl gets initialized, it turns the tracking of because of
in
GetMemoryDebug()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the comment. Based on the feedback I specifically removed the option to enable the detailed tracking via environment to avoid cases when managed code may be invoked on incompatible thread. What remains is ability to subscribe for the detailed reporting later (when all the initialization is done) when caller feels it is safe. I know this part may be tricky to describe. But so far I failed to construct case where it failed e.g. all the cases I was interested in so far just worked and provided the info I was looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following program will not print any memory addresses, although one would expect that it should
It starts to work if you uncomment the first two lines.