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

Consider debug mode for ArrayPool #7532

Open
danmoseley opened this issue Mar 1, 2017 · 6 comments
Open

Consider debug mode for ArrayPool #7532

danmoseley opened this issue Mar 1, 2017 · 6 comments
Labels
area-System.Runtime enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@danmoseley
Copy link
Member

danmoseley commented Mar 1, 2017

There are several ways to misuse array pool

  1. forget to return - merely inefficient
  2. return and continue to use
  3. renter expects pre cleared array
  4. returner does not request a clear and leaks sensitive data
  5. double return

There is some logging to help trace suspected cases of 1 and 2.

For 4 if we start caring more about that someday we could change the default to clear data.

For 5 we could at least (in a diagnostic mode) compare against existing arrays in the pool (perhaps an expanded size pool that returns LRU so it's likely still in the pool when returned the 2nd time)

We could help catch cases of both 2 and 3 by having a mode where we set the array to 0xDEADBEEF on return. We would do this whether or not the returner requested the array clear, because the next renter cannot rely on the returner.

This should be enabled in #if DEBUG but since running chk corelib is so rarely done, it might realistically only get used if it was #if DEBUG || DEBUG_ARRAYPOOL or something like that. So perhaps this isn't worth bothering with unless and until we have some investigation that it would help with.

Just a thought for the future.

@danmoseley
Copy link
Member Author

cc @stephentoub

@jkotas
Copy link
Member

jkotas commented Mar 2, 2017

Requiring a different build of the runtime to do this is non-starter. This would need to be enabled dynamically with a config switch or environment variable. Similar to how malloc/free diagnostic works in C++ today (pageheap on Windows, MALLOC_CHECK_ on Linux, etc.)

@danmoseley
Copy link
Member Author

Oh - it's right in front of my nose in PinnableBufferCache already. Just another one.

#if ENABLE
            // Check to see if we should disable the cache.
            string envVarName = "PinnableBufferCache_" + cacheName + "_Disabled";
            try
            {
                string envVar = Environment.GetEnvironmentVariable(envVarName);
                if (envVar != null)

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@danmoseley
Copy link
Member Author

Another option would be a different factory method to create the ArrayPool (perhaps overloads or instead of Shared() or Create()) that would return the diagnostic pool. Presumably there are relatively few places in one's code where the array pool is created (probably 1, if you know which array pool is leading to the problem) so changing that one line of code for testing purposes is not onerous. As a separate concrete implementation of ArrayPool performance would not be a concern and it could record callstacks, etc like a diagnostic heap.

@huoyaoyuan
Copy link
Member

Can this be achieved with AppContext switch? Since the checks may be somehow intensive, I'm not sure about Tier-0 performance impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

6 participants