Feat/log on closed connections #123
Conversation
|
I don't think writableEnded is there in node v10. |
|
@mcollina: I verified this fails on Node 10 locally and passed up to 15. I think that you're on to something. |
|
@mcollina: I reviewed (and you reviewed apparently :D) the PR where I wasn't sure if we exited early based on the order of events firing, so I removed my listener tweak before I got started. I tried to backport this to Node.js 10 using |
jmealo
force-pushed
the
feat/log_on_closed_connections
branch
3 times, most recently
from
aafe442
to
a473a33
Compare
|
@mcollina: I think this works best. Let me know what you think. |
jmealo
force-pushed
the
feat/log_on_closed_connections
branch
from
a473a33
to
d1afd5b
Compare
|
@mcollina: Here's what I landed on: Behavior: Reasoning: |
| var line = dest.read() | ||
| t.equal(line, null) | ||
| t.end() | ||
| }, 100) |
There was a problem hiding this comment.
Can you use a setImmediate() instead?
There was a problem hiding this comment.
Hm... The tests were the only thing I didn't rewrite; I can take a look at these later today. I don't see why not.
There was a problem hiding this comment.
@mcollina: I wasn't able to completely replace setTimeout with setImmediate without a triple nested setImmediate which seemed worse from a clarity standpoint. The author's original intent was to give the request enough time to start so a failure could be simulated and ultimately logged. One or two of these setTimeout can be easily replaced with setImmediate. To completely remove setTimeout would require more work than I can set aside right now. I may be able to revisit later -- or anyone else is free to pick this up as well! It's a good task for those looking to learn about the particulars of the event loop.
There was a problem hiding this comment.
using setTimeout will lead to flaky CI runs, I really need this fixed before landing.
Another approach is using some logical events instead of waiting.
| var line = dest.read() | ||
| t.equal(line.finished, false, 'unfinished request is logged and flagged') | ||
| t.end() | ||
| }, 100) |
There was a problem hiding this comment.
Can you use a setImmediate() instead?
| @@ -15,6 +16,9 @@ function setup (t, logger, cb, handler) { | |||
| if (req.url === '/') { | |||
| res.end('hello world') | |||
| return | |||
| } else if (req.url === WAIT_URL) { | |||
| setTimeout(() => res.end('that was boring'), 300) | |||
There was a problem hiding this comment.
why a setTimeout? Can you use a setImmediate() instead?
|
closed for no activity |
This PR fixes the git oops in #117 authored by @dalexanco
Minor change:
I decided not to attach/detach a listener for the
finishevent when we don't plan on logging it. This shouldn't leave any dangling handlers or change the intended behavior.Tests pass.