`warp`-based server fails with errno 104 when using `time-manager-0.3.0`, but not with `time-manager-0.2.4`

[RyanGlScott]

I am debugging a warp-based server application that receives RPC commands over an HTTP connection. We noticed that when upgrading from time-manager-0.2.4 to 0.3.0, many of the tests began to fail with errno 104 (Connection reset by peer), as seen in GaloisInc/argo#227. These errors do not occur when using time-manager-0.2.4.

Unfortunately, I'm not really sure how to debug this further just yet, as time-manager is rather deep in the implementation details of warp. Does this sort of error give you any ideas as to what might be going wrong? Is this is a bug in time-manager/warp, or do we need to be using warp in a different way post-time-manager-0.3.0?

[Vlix]

@kazu-yamamoto has more knowledge of networking quirks and what time-manager is specifically used for, but AFAIU the main point of the time-manager in warp is to implement the anti-"Slow Loris" functionality.

IIUC, you have confirmed that the test runs with 0.2.4 and fails with 0.3.0. A few questions:

• Does the failing/succeeding happen consistently, or is it flaky?
• Does this happen when sending the request or receiving the response? (and is that consistent?)
• Does this happen when using HTTP/1.1 or HTTP/2? (and if you can easily test: does it happen with both?)

A possibility I can think of why you're now getting this error is the time-manager update is slightly more precise in its timing, so you MIGHT be hitting the timeout barely, which with the 2.4.0 version you wouldn't. (but the inprecision should be random, so if you NEVER get this error with 2.4.0, then I doubt this is the case)

More likely, though, is that there's a slight change in processing of the time-manager we've overlooked.

[Vlix]

@kazu-yamamoto:

After some codebase scouring in the warp modules, I found one potential spot.

The resume logic is the only logic that really changed in between 0.2.4 and 0.3.0, I think. It used to be a synonym for tickle, but now it's a register; this makes sense for the pause/resume duality, but the moment it's being used like a tickle it will overwrite the handleKeyRef :: IORef. This happens at one point in warp in the warp/Network/Wai/Handler/Warp/HTTP1.hs module at line 233 & 241.

I think we have to either:

• make the resume work like a tickle if the Handle hasn't been paused, or
• we need to be more specific in the documentation of resume that you should NOT use it as a tickle AND replace the resumes in HTTP1.hs with tickles.

What do you think?

(@RyanGlScott, if it happens when using HTTP2, then this either isn't the issue, or it's not the whole issue)

[RyanGlScott]

Ah, I was previously running the application in a way that suppressed certain errors that GHC produces at runtime, which made me overlook this very relevant error message:

GHC.Event.Thread.getSystemTimerManager: the TimerManager requires linking against the threaded runtime
CallStack (from HasCallStack):
  error, called at libraries/base/GHC/Event/Thread.hs:275:13 in base:GHC.Event.Thread

And indeed, I was not compiling my application with -threaded! If I do, then the issues I was experiencing go away.

I'm not sure if there is an implicit requirement to use -threaded or not. If there is, perhaps it would be worth documenting it explicitly?

[Vlix]

@kazu-yamamoto I think we're going to need to serve either the 0.2.4 version or the 3.0.0 version based on whether GHC is threaded or not 🤔

The GHC.Internal.Event(.*) modules don't have much way of checking whether the RTS is threaded or nut, but one of the ways is to use getSystemEventManager (which will be Nothing if the RTS isn't threaded)
And if we're going to allow both types, we might as well also check for if the compiler is GHC or not, to allow non-GHC compilers to use the 0.2.4 version, maybe?

[kazu-yamamoto]

According to the following signatures, I understood that non-threaded GHC should provide SystemTimerManager, too.

getSystemEventManager :: IO (Maybe EventManager)
getSystemTimerManager :: IO TimerManager

I need to understand why this assumption is not correct.

[kazu-yamamoto]

Oh my...

getSystemTimerManager :: IO TM.TimerManager
getSystemTimerManager =
  fromMaybe err `fmap` readIORef timerManager
    where
      err = error "GHC.Event.Thread.getSystemTimerManager: the TimerManager requires linking against the threaded runtime"

Really bad APIs.

[kazu-yamamoto]

Are there any users to implement network code with non-threaded GHC?
If not, this is just a document issue.

[Vlix]

Really bad APIs.

Yes, I also expected it to be fine.

I don't feel like it's guaranteed that someone would only use the System.TimeManager with a -threaded runtime, so we should AT LEAST add a large disclaimer to the documentation if we don't change the functionality.

I do think that resume should either be adjusted to also function as a tickle, if called more than once, or that we at least add a warning that resume does NOT equal a tickle.

[kazu-yamamoto]

I do think that resume should either be adjusted to also function as a tickle, if called more than once, or that we at least add a warning that resume does NOT equal a tickle.

I think the former is better but don't know how to implement it?
updateTimeout just returns () even if the entry is already gone.