Koa: Sometimes request.ip returns undefined

[RHavar]

After certain types of errors (e.g. { [Error: Parse Error] bytesParsed: 159, code: 'HPE_INVALID_METHOD' }) the remote socket gets closed, and calling .remoteAddress on it will return undefined.

Because koa's request.ip is lazy, this means it's sometimes returning undefined which screws up some things (e.g. I record errors in a format that requires an ip, and would like to see the ip address of the bad requests).

I think this is quite unexpected behavior, and would be more sensible to strictly evaluate .ip to always be available

[luckydrq]

Can you post an example? I think request.ip can be accessed during the whole request life cycle.

[dougwilson]

Yes, .remoteAddress will become undefined when the socket is no longer active: https://nodejs.org/api/net.html#net_socket_remoteaddress

The reason is that calling that getter calls into C++ and does a socket operation to get this information. There is a proposal somewhere in Node.js itself to cache this value instead, making it available to libraries like Koa.

[kraghack]

Does Koa have to preserve this unexpected/bug-prone behavior just because Node has it?

[PlasmaPower]

I'd be in favor of caching the value. It could be set in createContext. The request IP shouldn't be changing.

[PlasmaPower]

Well _someone_ likes emotes a lot _(glares at @RHavar)_

[RHavar]

I'd be in favor of caching the value. It could be set in createContext. The request IP shouldn't be changing.

Seems like the most reasonable solution. I quickly mocked this up, and don't see any problems. The only apparent user-visible difference seems to be request.ip would now become reassignable while currently it's not (unless using object.assign ). However this is probably a good thing, as it would be easier for some middleware to override the ip (e.g. some middleware wants to reassign request.ip based on the value from the header cf-connecting-ip when behind cloudflare)

As luckydrq's comment shows the current behavior is pretty unexpected, so I think it's something that should be fixed

[PlasmaPower]

I'm making a PR now, it's almost done

[luckydrq]

@dougwilson thanks for explanation. @RHavar I will look deeper into that, also thanks for this issue.

[jonathanong]

should be solved by https://github.com/koajs/koa/issues/698. open a new issue if you disagree :)

[PlasmaPower]

@jonathanong I need to remember to put "Fixes #issue" in my commit messages :)

[jonathanong]

oops forgot to close.

i missed it before github let you merge + squash because i did it manually for a while and github didn't automatically close