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 nil pointer dereference on pipe close #45

Closed
wants to merge 1 commit into from

Conversation

m-kostrzewa
Copy link

Fixes #44

We don't nil the file pointer until after the connectPipe() function returns.

@dgageot
Copy link

dgageot commented Apr 26, 2017

Hi @m-kostrzewa, do you think it's possible to write a unit test that demonstrate the issue?

@m-kostrzewa
Copy link
Author

m-kostrzewa commented Apr 26, 2017

Hi @dgageot

I don't see a way to do this without introducing mocks, and I don't see them being used it current test suite. I could theoretically write a test that spawns like 1000 goroutines and detect the race that way, but it would be flaky and not really a unit test anymore. Not to mention it wouldn't really demonstrate the issue.

I wonder why this Sleep exists: https://github.com/Microsoft/go-winio/blob/master/pipe_test.go#L172. Maybe it was due to flaky test?

@m-kostrzewa
Copy link
Author

@dgageot I asked some of my friends and they're not sure how to demonstrate this issue with unit tests either... it's a race condition, after all.

For what it's worth, we merged this patch to our repos 1.5 months ago and the issue never appeared again, and it would happen every other test run before that.

@jstarks
Copy link
Member

jstarks commented Aug 2, 2017

Thanks for the contribution, sorry for the delay. We decided to take PR #63, which should address the same issue.

@jstarks jstarks closed this Aug 2, 2017
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.

3 participants