Addition of setInitHandler #24

Merged
merged 5 commits into from May 29, 2012

Conversation

Projects
None yet
3 participants
Contributor

ndmitchell commented May 25, 2012

I am writing something which needs to spawn a web server, wait until it is ready to receive connections, and then tell someone the port. There are two related problems:

  • The server does not say when it has started
  • If you set the port to be 0, then a free port is allocated - there is no easy way to see which port the server ended up on.

This patch solves both these issues with an initial handler, which is run after the server is started, and gets passed the real sockets, which you can call socketPort on to find which port the server is running on.

Neil Mitchell Add a configuration function setInitHandler, which provides a handler…
… that is run after all sockets have been connected and the server is ready to receive input
fc0af58
Member

cdsmith commented May 25, 2012

I think this is a great idea. I needed the same thing, and solved it by spawning a thread that sits in a loop just trying to connect and then waiting 50 ms, repeat until it succeeds. This is a much cleaner answer.

Contributor

ndmitchell commented May 25, 2012

Note that for my case even polling wasn't sufficient (which forced me to do it properly), as as if you pass a port of 0, then you can't figure out the port the server starts on at all.

@gregorycollins gregorycollins commented on an outdated diff May 28, 2012

src/Snap/Http/Server/Config.hs
@@ -41,6 +41,7 @@ module Snap.Http.Server.Config
, getSSLKey
, getSSLPort
, getVerbose
+ , getInitHandler
@gregorycollins

gregorycollins May 28, 2012

Owner

I think "InitHandler" would be better renamed "StartupHook" here.

@gregorycollins gregorycollins commented on an outdated diff May 28, 2012

src/Snap/Http/Server/Config.hs
@@ -141,6 +144,7 @@ data Config m a = Config
, other :: Maybe a
, backend :: Maybe ConfigBackend
, proxyType :: Maybe ProxyType
+ , initHandler :: Maybe (Config m a -> [Socket] -> IO ())
@gregorycollins

gregorycollins May 28, 2012

Owner

I'm not sure about the type here. Are there other things besides that list of sockets that a person writing a hook here might be interested in? Will there ever be? If so, I'm going to have to go through a deprecation cycle in order to change this method without breaking user code. Perhaps a more future-proof way to do this is to define an opaque datatype for the arguments, i.e.:

, startupHook :: Maybe (StartupHookData m a -> IO ())

We should then be able to make more arguments available to these hooks without going through a deprecation cycle.

Owner

gregorycollins commented May 28, 2012

Overall: Neil, thanks for doing this. Besides the naming changes I commented on above, it's fine with me. The only other thing I wanted to ask is: have you checked the testsuite? Without changes to match there I expect this changeset probably breaks the build.

Contributor

ndmitchell commented May 28, 2012

All good points. I hadn't realised the test suite dipped into the internals, I'll include a patch fixing that up.

Contributor

ndmitchell commented May 28, 2012

I think that's all the points addressed, let me know if you have any further comments.

@gregorycollins gregorycollins commented on an outdated diff May 29, 2012

src/Snap/Http/Server/Config.hs
+setStartupHook :: (StartupHookData m a -> IO ()) -> Config m a -> Config m a
+setStartupHook x c = c { startupHook = Just x }
+
+
+------------------------------------------------------------------------------
+
+-- | Arguments passed to 'setStartupHook'.
+data StartupHookData m a = StartupHookData
+ { startupHookConfig :: Config m a
+ , startupHookSockets :: [Socket]
+ }
+
+emptyStartupHookData :: StartupHookData m a
+emptyStartupHookData = StartupHookData emptyConfig []
+
+-- | The the 'Socket's openned by the server. There will be two 'Socket's for SSL connections, and one otherwise.

@gregorycollins gregorycollins commented on the diff May 29, 2012

src/Snap/Http/Server/Config.hs
+ { startupHookConfig :: Config m a
+ , startupHookSockets :: [Socket]
+ }
+
+emptyStartupHookData :: StartupHookData m a
+emptyStartupHookData = StartupHookData emptyConfig []
+
+-- | The the 'Socket's openned by the server. There will be two 'Socket's for SSL connections, and one otherwise.
+getStartupHookSockets :: StartupHookData m a -> [Socket]
+getStartupHookSockets = startupHookSockets
+
+-- The 'Config', after any command line parsing has been performed.
+getStartupHookConfig :: StartupHookData m a -> Config m a
+getStartupHookConfig = startupHookConfig
+
+setStartupHookSockets :: [Socket] -> StartupHookData m a -> StartupHookData m a
@gregorycollins

gregorycollins May 29, 2012

Owner

I think we don't need setters here in the public API, but I'll refactor this later.

@ndmitchell

ndmitchell May 29, 2012

Contributor

I considered that, but to avoid setters in the public API you'd need to move all of Config into somewhere internal, and reexport it from here. I thought that was a bit of a big hammer to take to someone elses code :)

Owner

gregorycollins commented May 29, 2012

Hey Neil, I'll either merge this later tonight and fix the typo myself or wait for you to fix it, whoever gets to it first.

Contributor

ndmitchell commented May 29, 2012

The typo fix is already in, I look forward to the merge.

@gregorycollins gregorycollins added a commit that referenced this pull request May 29, 2012

@gregorycollins gregorycollins Merge pull request #24 from ndmitchell/master
Addition of setInitHandler
41ffbf2

@gregorycollins gregorycollins merged commit 41ffbf2 into snapframework:master May 29, 2012

Owner

gregorycollins commented May 29, 2012

I just committed 77fadeb which cleaned up some of the stuff we talked about. I also renamed StartupHookData with something shorter after thinking about it a little bit, sorry.

Contributor

ndmitchell commented May 30, 2012

Thanks for the commit and cleanup - looks very good - much appreciated.

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