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 support for windows thread block hack and closeOnExec to TLS. #674

Merged
merged 1 commit into from Feb 19, 2018

Conversation

Projects
None yet
3 participants
@ondrap
Contributor

ondrap commented Feb 15, 2018

Before submitting your PR, check that you've:

On Windows the TLS web server cannot be killed; this is especially problematic if you run it as windows service and it cannot be stopped by normal means. It seems that some things that are done when opening the normal runSettings from warp are not performed in runTLS. This patch sets CloseOnExec flag on the socket and uses the windows hack to let the thread to be killed.

@snoyberg

LGTM, thanks! @kazu-yamamoto any objections?

@snoyberg snoyberg requested a review from kazu-yamamoto Feb 18, 2018

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Feb 19, 2018

@ondrap This patch is really nice. Thank you!

@snoyberg I dislike that this kind of PRs includes version-bump. What do you think?

@snoyberg

This comment has been minimized.

Member

snoyberg commented Feb 19, 2018

I know that opinions on this go both ways, but I've encouraged contributors to include version bumps, see the second bullet on https://www.snoyman.com/blog/2017/06/how-to-send-me-a-pull-request. I'm fine with running Warp differently, we should probably include that in a CONTRIBUTING.md file.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Feb 19, 2018

@snoyberg I read it. But I don't see why you like it. What if multiple PRs come in a short period?

@snoyberg

This comment has been minimized.

Member

snoyberg commented Feb 19, 2018

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Feb 19, 2018

I understand. Let's go with your way.

And this PR is perfectly OK with me.

@kazu-yamamoto

LGTM.

@snoyberg snoyberg merged commit 2cec999 into yesodweb:master Feb 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snoyberg

This comment has been minimized.

Member

snoyberg commented Feb 19, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment