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

runTLSSocket and runSettingsSocket functions have different behavior. #361

Closed
TravisWhitaker opened this issue Apr 7, 2015 · 22 comments

Comments

@TravisWhitaker
Copy link

commented Apr 7, 2015

I've been using constructs a bit like the following to implement graceful shutdown initiated by an MVar:

safeShutdown :: MVar () -> MVar () -> Socket -> IO ()
safeShutdown init_term warp_dead sock = do
    takeMVar init_term
    close socket
    takeMVar warp_dead

-- Assumes warp is invoked with something like this:

let ws = setPort 10000 defaultSettings
s <- bindPortTCP (getPort ws) (getHost ws)
wd <- newEmptyMVar
forkFinally (runSettingsSocket ws s app) (const $ putMVar wd ())

Documentation for the 'runSettingsSocket' function states that "When the listen socket in the second argument is closed, all live connections are gracefully shut down." The idea is to close the listening socket, then wait for the warp thread to exit. And this works perfectly in the above case. However, if 'runTLSSocket' from 'warp-tls' is used:

let ws = setPort 10000 defaultSettings
let ts = defaultTlsSettings {certFile = "cert", keyFile = "key"}
s <- bindPortTCP (getPort ws) (getHost ws)
wd <- newEmptyMVar
forkFinally (runTLSSocket ts ws s app) (const $ putMVar wd ())

In this case 'safeShutdown' seems to block indefinitely on the warp thread exiting. I noticed a few things comparing the 'runTLSSocket' and 'runSettingsSocket' functions. 'runSettingsSocket' calls the 'settingsInstallShutdownHandler' function, while runTLSSocket seems to ignore it. 'runTLSSocket' calls 'runSettingsConnectionMakerSecure' directly, while 'runSettingsSocket' involves a call to the 'runSettingsConnection' function, which in turn calls the 'runSettingsConnectionMaker', which finally calls the 'runSettingsConnectionMakerSecure' function. I suspect that this is the cause of the differences in behavior between the 'runSettingsSocket' and 'runTLSSocket' functions, principally:

  • 'runSettingsSocket' (and functions that eventually call it, like 'run') calls the shutdown handler in the warp settings object while 'runTLSSocket' (and functions that eventually call it, like 'runTLS') silently ignore it.
  • If the socket passed into 'runSettingsSocket' is closed, the warp thread gracefully terminates. If the socket passed into 'runTLSSocket' is closed, the thread waits forever after the last active client disconnects.

I haven't dug into the warp main loop enough to understand exactly what is going on(maybe this is a good excuse to do so), but I feel as though these two functions should have the same behavior, unless of course I'm missing something here.

@snoyberg

This comment has been minimized.

Copy link
Member

commented Apr 8, 2015

I'm not certain (the code you provided seems to have some typos and is using the wrong function names in each snippet), but I think you're reporting a real issue. My guess is that your analysis is in fact correct. Since you're currently reproducing this, would you be able to take a stab at tweaking warp-tls to respect the necessary flags?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2015

@TravisWhitaker Your observation is correct. Please read acceptConnection in Run.hs.

When a listen socket is closed, acceptLoop returns and gracefulShutdown is called. So, a listen socket must be closed for graceful shutdown.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2015

settingsInstallShutdownHandler is used to install a handler to shutdown a listen socket, for instance, for SIGHUP. In your case, safeShutdown closes a listen socket. So, settingsInstallShutdownHandleris not related to your issue.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2015

I guess that try getConnMaker does not return Left in TLS even if a listen socket is closed.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2015

@snoyberg runTLSSocket should call settingsInstallShutdownHandler anyway.

@snoyberg snoyberg added the ready label Apr 21, 2015

@TravisWhitaker

This comment has been minimized.

Copy link
Author

commented Apr 21, 2015

Sorry for the delay, I've been busy with a few other projects and this issue wasn't a showstopper. I'll pretty up my branch and send a pull request as soon as I get a few free hours.

@kazu-yamamoto is correct, the connection maker set up by runTLSSocket' doesn't return Left even if its socket is closed.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2015

We need to ask the tls maintainer that this is a bug or a spec.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented May 13, 2015

I think I understand the source of this problem. getter in WarpTLS.hs uses acceptSafe which is imported Data.Streaming.Network. This function never ends: https://hackage.haskell.org/package/streaming-commons-0.1.0.0/docs/src/Data-Streaming-Network.html#acceptSafe

@snoyberg We should do two things:

  • WarpTLS should use accept to share the accept logic with Warp if you don't have strong reason to use acceptSafe.
  • acceptSafe should steal isFullErrorType (ioeGetErrorType e) from Warp's Run.hs to loop only when "Too many open files" happens.

@TravisWhitaker Could you modify WarpTLS so that it uses accept and test it?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

Pinging @snoyberg.

snoyberg added a commit to fpco/streaming-commons that referenced this issue May 15, 2015
@snoyberg

This comment has been minimized.

Copy link
Member

commented May 15, 2015

@kazu-yamamoto is fpco/streaming-commons@f3d5cd2 what you were getting at?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

Yes!

@snoyberg

This comment has been minimized.

Copy link
Member

commented May 15, 2015

OK, released. Does this mean @TravisWhitaker should be able to test with the new version of streaming-commons (0.1.12.1)?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

@snoyberg We should use accept instead of acceptSafe in WarpTLS to avoid the logic overlapping.

@snoyberg

This comment has been minimized.

Copy link
Member

commented May 15, 2015

Why not have warp use acceptSafe instead?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

Simply replacing accept with acceptSafe in Run.hs does not make sense to me. Would you show me the code which you intend?

@snoyberg

This comment has been minimized.

Copy link
Member

commented May 15, 2015

That is what I intended. I think I'm misunderstanding what's going on here. Is it intentional for accept from warp and acceptSafe from streaming-commons to have different behavior?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

Please give a look at acceptNewConnection in Run.hs. This code includes isFullErrorType (ioeGetErrorType e) in the different level of acceptSafe. To share this code, I think we should replace acceptSafe to accept in WarpTLS.

@snoyberg

This comment has been minimized.

Copy link
Member

commented May 17, 2015

OK, I understand what you're saying now. When you said "use accept to share logic with warp", I thought you meant using a function from warp, not using the standard accept. I understand now that warp is already applying that logic to the function in warp-tls.

So to confirm: you mean to replace acceptSafe with Network.Socket.accept in warp-tls, and warp will handle the necessary retry logic by wrapping that call to accept with acceptConnection (Run.hs line 161), correct?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented May 18, 2015

Yes, exactly. Sorry for my poor explanation.

snoyberg added a commit that referenced this issue May 18, 2015
@snoyberg

This comment has been minimized.

Copy link
Member

commented May 18, 2015

OK, change pushed. Unless there are objections I can release this.

eed0bf9

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented May 18, 2015

@TravisWhitaker Could you check if the patch above solves your problem?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2015

I would close this. Please reopen if necessary.

@snoyberg snoyberg removed the ready label Jun 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.