Conversation
|
Hmmm. I'm a bit lost here. I've transmuted this issue into a PR to share my current debugging process. Below the output from the test run of Handle Any idea??? Please ???? |
|
Ok. I'm completely lost now. Or maybe a tad underslept. I've added some more tracing around the building/disposing of the handle and some expected output is not executed. I need another pair of 👀 here. |
There was a problem hiding this comment.
Those two lines above do not appear in the output
|
facepalm I've found something. Sorry for the noise.... |
|
Ok. A first batch of leaks have been plugged. Some remain though. Working on plugging them. Still, I'd be interested in failing the test suite when some are detected. Ideas? |
|
Okay so, my idea would be the following:
|
|
@Therzok 👍 You made my day! |
|
@Therzok Here's what I came with. with. Unfortunately, throwing from the |
|
Okay, so including the comment above, why not use Debug.Fail instead of an exception? |
|
Want me to jump in on the PR and do the changes so it's easier to understand after? |
|
I've tried different approaches (Environment.FailFast, trying to output the messages, ...) without much success. FailFast was crashing the process but the Console buffer wasn't flushed so no useful information for the user.... Latest one is to make the test suite monitor some shared container for reported leaked handles and fail the current test. Works on AppVeyor but completely ignored on Travis 😕 I didn't think of Debug.Fail() though. Would it output something in Travus/AppVeyor output?
Yes, please!!!! I think I can't think straight any longer for now. ❤️ |
|
@Therzok The PR is yours, Sir! |
There was a problem hiding this comment.
@Therzok If you think going down this path makes sense (and we actually succeed in making it work on Travis), maybe should we clear the LeaksContainer once we've failed the test in order to avoid polluting (too much) the build log.
There was a problem hiding this comment.
Actually, I'm not getting anything useful when defining LEAKS_IDENTIFYING on Mac. Hmmm, something is stale over here.
|
Okay, will do. Starting on it now. |
|
Actually, depending on XUnit on this thing is way better for unit tests. |
|
|
|
I would like to do one more fixup. We need to split each define into a separate msbuild property. i.e. calling build.libgit2sharp.cmd with 'LEAKS_TRACKING;LEAKS_IDENTIFYING' won't work. regex_replace("([a-zA-Z_]+)", \1=\1) |
|
@nulltoken you made some manual changes to the AppVeyor build and that keeps your old behaviour on the msbuild targets. |
|
On mono, disposing is always true in BaseFixture. Still trying to dig it down. Switched from ConcurrentBag to HashSet since we're doing locks. ConcurrentBag is unstable from what I read on the internets. |
8e0ae9a to
6139777
Compare
|
@dahlbyk @ethomson @jamill @carlosmn Ready for review, I think... Unless @Therzok is willing to polish this further. This will fail the AppVeyor build when leaked handled are detected.
|
|
As I said, my only remaining improvement is to split ExtraDefines into LeaksTracking and Leaks Identifying, since msbuild doesn't seem to support multiple values in the parameter. I.e. we can't do |
|
That is, if we ever end up wanting to define more than one extra symbol. |
|
Let's just merge as-is, imo. Other than "ExtraDefines" to "ExtraDefine" or per-define specific property, I have nothing else to add. I can go either route if it's wanted. |
|
Renamed to ExtraDefine. |
|
You forgot CI/build.msbuild changes. |
This is why I pushed the fix in the first place. I knew I was going to forget something 😉 |
|
Ok. Cleaning up the PR, now... |
|
Edit: My failed attempt at making a gif. |
|
Hmmm. Something's fishy here. This should have failed. There are still some purposefully injected leaks |
|
Where did I mess up? |
|
Found. I've forgot to change the AppVeyor settings. |
|
Ok. AppVeyor detected the leak. Now let's drop the injection of fake leaks |
|
@Therzok I'd like to thank you very much with the help you've provided with nailing this. I'm pretty psyched about this! We should now be protected about future leaks! 💯 |
|
@nulltoken it was my pleasure to have this handled! ❤️ I've pushed a bug report about SafeHandle issues on Mono. Luckily, AppVeyor runs after travis so we don't miss results. |
|
SafeHandle is now fixed in mono master. |
|
Wow! That was fast! |
2 participants
I've recently repaved my box and switched back from the Resharper test runner to Testdriven.net.
And... we're leaking handles. The issue with that is that the dll gets locked and cannot be rewritten over. That would correlate with some reports I've had of such issues from @Allov and @davidebbo.
For instance
UnstageFixture.UnstagingUnknownPathsWithStrictUnmatchedExplicitPathsValidationThrows()leaks oneDiffSafeHandle.I'm currently investigating this further and trying to identify and fix those leaks.
However, I'd be a taker if you had an idea regarding how we could abort the test suite when that happens.