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

Automatically call withSocketsDo everywhere #107

Closed
ndmitchell opened this Issue Feb 25, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@ndmitchell
Contributor

ndmitchell commented Feb 25, 2015

Currently, users must call withSocketsDo before using the http library. Likely that was because people assumed it to be expensive, or didn't know how it composed. We now know it isn't expensive, and the rules on how it composes are incredibly flexible: http://neilmitchell.blogspot.co.uk/2015/02/making-withsocketsdo-unnecessary.html

I suggest a call to withSocketsDo around each high level operation such as withManager (not sure if you're going to need it around parseUrl, but you should probably check). If you worry about the performance, I suggest you follow the CAF/unsafePerformIO/evaluate trick from the blog post above, but I doubt its going to be any noticeable performance on any version.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 26, 2015

Owner

Sounds good to me. I'll try to look at this some time next week.

On Thu Feb 26 2015 at 12:22:24 AM Neil Mitchell notifications@github.com
wrote:

Currently, users must call withSocketsDo before using the http library.
Likely that was because people assumed it to be expensive, or didn't know
how it composed. We now know it isn't expensive, and the rules on how it
composes are incredibly flexible:
http://neilmitchell.blogspot.co.uk/2015/02/making-withsocketsdo-unnecessary.html

I suggest a call to withSocketsDo around each high level operation such
as withManager (not sure if you're going to need it around parseUrl, but
you should probably check). If you worry about the performance, I suggest
you follow the CAF/unsafePerformIO/evaluate trick from the blog post above,
but I doubt its going to be any noticeable performance on any version.


Reply to this email directly or view it on GitHub
#107.

Owner

snoyberg commented Feb 26, 2015

Sounds good to me. I'll try to look at this some time next week.

On Thu Feb 26 2015 at 12:22:24 AM Neil Mitchell notifications@github.com
wrote:

Currently, users must call withSocketsDo before using the http library.
Likely that was because people assumed it to be expensive, or didn't know
how it composed. We now know it isn't expensive, and the rules on how it
composes are incredibly flexible:
http://neilmitchell.blogspot.co.uk/2015/02/making-withsocketsdo-unnecessary.html

I suggest a call to withSocketsDo around each high level operation such
as withManager (not sure if you're going to need it around parseUrl, but
you should probably check). If you worry about the performance, I suggest
you follow the CAF/unsafePerformIO/evaluate trick from the blog post above,
but I doubt its going to be any noticeable performance on any version.


Reply to this email directly or view it on GitHub
#107.

@DanBurton

This comment has been minimized.

Show comment
Hide comment
@DanBurton

DanBurton Mar 9, 2015

Contributor

I realize that the motivation to continue providing withSocketsDo is for backwards compatibility, but at this point, shouldn't it be deprecated in favor of something like

{-# INLINE initSockets #-}
initSockets :: IO ()
#if !defined(WITH_WINSOCK)
initSockets = return ()
#else
initSockets = evaluate withSocketsInit

Since we're no longer calling shutdownWinSock, there doesn't seem to be any point in retaining the "bracket" call pattern of withSocketsDo. If I'm not mistaken, all network activity in http-client requires a manager (either explicitly or under the hood), so it should be sufficient to initSockets sometime during newManager.

That said, I think it would be better to go with Simon Marlow's suggestion to have the initialization happen automatically and leave this stuff out of http-client entirely.

wrt people using old libraries but wanting to program in the new style of not worrying about withSocketsDo themselves, I suppose we could stick a withSocketsDo around withManager. However, I'm concerned: does the old withSocketsDo nest safely?

Contributor

DanBurton commented Mar 9, 2015

I realize that the motivation to continue providing withSocketsDo is for backwards compatibility, but at this point, shouldn't it be deprecated in favor of something like

{-# INLINE initSockets #-}
initSockets :: IO ()
#if !defined(WITH_WINSOCK)
initSockets = return ()
#else
initSockets = evaluate withSocketsInit

Since we're no longer calling shutdownWinSock, there doesn't seem to be any point in retaining the "bracket" call pattern of withSocketsDo. If I'm not mistaken, all network activity in http-client requires a manager (either explicitly or under the hood), so it should be sufficient to initSockets sometime during newManager.

That said, I think it would be better to go with Simon Marlow's suggestion to have the initialization happen automatically and leave this stuff out of http-client entirely.

wrt people using old libraries but wanting to program in the new style of not worrying about withSocketsDo themselves, I suppose we could stick a withSocketsDo around withManager. However, I'm concerned: does the old withSocketsDo nest safely?

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Mar 9, 2015

Owner

but at this point, shouldn't it be deprecated in favor of something like

But that's exactly the point of backwards compatibility: we can't deprecate it, use something else, and be compatible with preexisting versions in the wild. Many things might be nicer, but the fact is that we're stuck with a version of network out there that provides withSocketsDo, so we have to use it.

withSocketsDo can be nested safely; have a look at the C functions.

Owner

snoyberg commented Mar 9, 2015

but at this point, shouldn't it be deprecated in favor of something like

But that's exactly the point of backwards compatibility: we can't deprecate it, use something else, and be compatible with preexisting versions in the wild. Many things might be nicer, but the fact is that we're stuck with a version of network out there that provides withSocketsDo, so we have to use it.

withSocketsDo can be nested safely; have a look at the C functions.

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell Mar 9, 2015

Contributor

You really want to ditch withSocketsDo entirely, not replace it with another API which also is prone to easy mistakes. Simon Marlow's suggestion of a constructor attribute is a nice idea, but in the past some people have had absolute disasters with that attribute. The next GHCi uses the system linker, so things might improve there, but for the moment, the constructor is just too dangerous.

All versions of withSocketsDo nest safely, and you are also free to do socket operations after the function returns. These facts are now documented in the HEAD network library, and it says they apply to old versions too.

Contributor

ndmitchell commented Mar 9, 2015

You really want to ditch withSocketsDo entirely, not replace it with another API which also is prone to easy mistakes. Simon Marlow's suggestion of a constructor attribute is a nice idea, but in the past some people have had absolute disasters with that attribute. The next GHCi uses the system linker, so things might improve there, but for the moment, the constructor is just too dangerous.

All versions of withSocketsDo nest safely, and you are also free to do socket operations after the function returns. These facts are now documented in the HEAD network library, and it says they apply to old versions too.

@DanBurton

This comment has been minimized.

Show comment
Hide comment
@DanBurton

DanBurton Mar 9, 2015

Contributor

Thanks for explaining that, @ndmitchell. It makes a lot more sense to me now. We'll go ahead and call withSocketsDo in newManager; it's an easy change that should add some nice backwards-compatible convenience for all http-client use cases.

Contributor

DanBurton commented Mar 9, 2015

Thanks for explaining that, @ndmitchell. It makes a lot more sense to me now. We'll go ahead and call withSocketsDo in newManager; it's an easy change that should add some nice backwards-compatible convenience for all http-client use cases.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Mar 11, 2015

Owner

@ndmitchell can you confirm that PR #110 fixes this?

Owner

snoyberg commented Mar 11, 2015

@ndmitchell can you confirm that PR #110 fixes this?

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell Mar 11, 2015

Contributor

Works for my example, seems very nice to me.

Contributor

ndmitchell commented Mar 11, 2015

Works for my example, seems very nice to me.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Mar 11, 2015

Owner

Cool, releasing to Hackage.

Owner

snoyberg commented Mar 11, 2015

Cool, releasing to Hackage.

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