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 super flaky tfdleak test on windows refs #14090 #14548

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jun 2, 2020

tfdleak is probably the most flaky test we have; this PR fixes that by establishing that isValidHandle is incorrect and ignoring when is returns true

I did some research (not being a windows user...), it's a a tricky topic but from what I gathered:

  • isValidHandle is incorrect on windows and has false positives
  • see also the test I added tests/stdlib/mfdleakIssue14090.nim which on windows will always fail (and always succeed on linux+osx)
  • trying to actually use the file handles (instead of relying on what isValidHandle says) indeed would give errors, indicating it hasn't really leaked but the handle could merely be reused in the child, with a different underlying resource
  • added links to relevant docs in the PR that talks about this

my "fix" is to disable the test in the case where a leak is not expected and windows sees a leak, since isValidHandle has false positives

future work

incorrect types should be fixed

# in lib/system/io.nim => bug
proc getOsfhandle(fd: cint): FileHandle {.importc: "_get_osfhandle", header: "<io.h>".}
# in lib/windows/winlean.nim => ok
proc get_osfhandle*(fd:FileHandle): Handle {.importc: "_get_osfhandle", header:"<io.h>".}

@alaviss
Copy link
Collaborator

alaviss commented Jun 2, 2020

this PR fixes that by establishing that isValidHandle is incorrect and ignoring when is returns true

I'm sorry, but this is not a fix.

trying to actually use the file handles (instead of relying on what isValidHandle says) indeed would give errors, indicating it hasn't really leaked but the handle could merely be reused in the child, with a different underlying resource

I'd be curious as to what is being used under that handle, maybe it's the various DLLs that's loaded by the program (due to winlean for example). The best way to check for this would be to purposefully leak a duplicated handle, then use CompareObjectHandles to verify whether the handle we are testing is the same as the handle held by the parent. I think I can try to implement this.

alaviss added a commit to alaviss/Nim that referenced this pull request Jun 2, 2020
Imported from nim-lang#14548 and tweaked for consumption by testament.

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
@timotheecour
Copy link
Member Author

timotheecour commented Jun 3, 2020

I'm sorry, but this is not a fix.

it's a fix for the flakiness but doesn't close #14090; the point was the test was flawed and needs a better test on windows; until then it doesn't make sense to have CI keep breaking; but if you have a better fix, I'm all for it

alaviss added a commit to alaviss/Nim that referenced this pull request Jun 3, 2020
Imported from nim-lang#14548 and tweaked for consumption by testament.

This test seems to be really good at bringing out the flakyness of
tfdleadk.

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
@Araq Araq closed this in #14550 Jun 4, 2020
Araq pushed a commit that referenced this pull request Jun 4, 2020
* tfdleak_multiple: introduce stress tester for tfdleak

Imported from #14548 and tweaked for consumption by testament.

This test seems to be really good at bringing out the flakyness of
tfdleadk.

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

* tfdleak: increase accuracy of the test on Windows

This commit implements a new testing strategy for Windows:
1. We duplicate the handle that will be tested and enable inheritance.
   This duplicate will serve as a reference handle.
2. In addition to checking whether the handle is valid, we also verify
   whether the handle is the same as the reference. This gives us
   complete certainty on whether the handle in question is inherited
   from the parent.
   A side effect is that this uses Windows 10+ APIs. But since
   this is just for the test, we don't have to be picky about it.

Ideally we would want to do something like this for other POSIX-based
system, but most of them lack a facility to do this, and as of writing
there isn't any false positive for them, so we won't need the additional
checks.

MemFile.fHandle will also no longer be tested, as this handle defaults
to being invalid.

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
@treeform
Copy link
Contributor

@timotheecour please help me I am getting this error, which you mentioned.

error C2664: 'BOOL SetHandleInformation(HANDLE,DWORD,DWORD)': cannot convert argument 1 from 'NI' to 'HANDLE'

Is there a way to fix that in nim? Is there a way to downgrade VC++ to a version that works? What would you recommend?

@treeform
Copy link
Contributor

A solution was found here: #14980

Thanks!

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