search

hisco / http2-client

Transparently make http request to both http1 / http2 server.
https://www.npmjs.com/package/http2-client
MIT License
33 stars 14 forks source link

Calling any stubbed method after the request has been made results in a stack overflow. #16

Closed SimonWoolf closed 5 years ago

SimonWoolf commented 5 years ago

Example of how to reproduce: set the timeout to something low and do req.on('timeout', () => req.abort()).

Lots of methods are stubbed, so they can be forwarded to the stream once the stream's actually been chosen. The problem comes if you call such a method after the stream has been chosen - for example, abort(), which forwards to close(). The stub calls this.genericStubber(), which checks for this[$stubs] -- which isn't present as it was set to null when the stream was take()n -- and defaults to just calling this[method]();. But that's just this.close(), which is the stub. So you get an infinite loop.

hisco commented 5 years ago

Hey, thanks for reaching out, I wasn't able to replicate the infinite loop you are describing. Although, I was able to find that if you call abort too fast, then this method would be unavailable...

If you could kindly share a code of the problem that I would be able to replicate this scenario.

Thanks!

hisco commented 5 years ago

BTW from the description you gave this is the error I was able to replicate an exception: https://runkit.com/embed/da1q3tmfwi8k

However, no infinite loop, I will be able to fix this exception but I want to be sure I see the full picture.

SimonWoolf commented 5 years ago

The exception you got is just because you're doing req1.setTimeout(1,(req)=>req.abort()), which is broken -- the setTimeout callback is just a listener for the timeout event, it isn't fed the request as an argument. If you replace it with req1.setTimeout(1,()=>req1.abort()), you'll see the stack overflow. https://runkit.com/simonwoolf/runkit-npm-http2-client

hisco commented 5 years ago

Sorry about my event handler mistake... middle of the night coding I wasn't seeing clearly =)

In any case I now see the bug... Thank you!

hisco commented 5 years ago

Fixed in #18 please close if agreed.

Thanks!

SimonWoolf commented 5 years ago

Don't think I agree it's fixed -- eg if you replace abort() with close() in the repro script you'll get the stack overflow again.

(But I'm no longer using this lib so have no vested interest here; up to you :slightly_smiling_face:)

hisco commented 5 years ago

close is not part of http1 interface it was wrongly added and now removed. Now we sync all public prototype functions of http1.