Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added connect.timeout() middleware #499

Closed
wants to merge 14 commits into
from

Conversation

Projects
None yet
2 participants

Uses new socket.setTimeout.

Tests included.

Keeps the options and defaults from the ancient connect-timeout middleware.

Just tried this branch with an actual project - routes that use res.render are working fine, but weirdly routes that use static files (via the static express middleware) generate timeouts. Ideas?

Chrome times out with .txt files, Firefox and Safari only time out with binary files like .jpg and .ico.

Perhaps I misunderstand how the socket detects idleness?

Hunter Loftis Added test for static middleware. I don't think .request() works the …
…same as a real browser, since safari/chrome/ff all timeout on certain static files.
e7b746f

@hunterloftis hunterloftis reopened this Mar 4, 2012

Member

tj commented Mar 23, 2012

hmm maybe we shouldn't use setTimeout after-all

It's socket-level rather than request-level... so different requests must be grouped into the same timeout unfortunately (that's the issue I was seeing with the eventemitter leaks).

Possibly the per-request setTimeout is a better way to go, ala Connect.timeout.

Member

tj commented Mar 25, 2012

right right, makes sense

Just did a quick test; one request failing can screw all the rests of the requests on the socket. Closing :/

Would you mind if I dropped something basically identical to connect-timeout into stock connect? I feel like it's a pretty fundamental capability of a request to timeout (I don't know anyone who doesn't use timeouts at least most of the time).

Member

tj commented Mar 26, 2012

yeah I'm down for that, I wanted it in a long time ago just never got around to it for some reason, but it's small so im +1

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