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

For discussion: Track and end sockets associated with clientKeys #6

Closed MikeRalphson closed 5 years ago

MikeRalphson commented 5 years ago

I'm seeing behaviour where programs hang once http or https requests are replaced with this library and no other changes are made.

Debugging it seems to point to TLSSockets being left open, so in this PR I've added explicit tracking and ending of those sockets.

I'd like your thoughts on this. Happy to fold the http2Sockets into a better structure with http2Clients maybe?

Bonus typo fixed too.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 30

2 of 11 (18.18%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-1.5%) to 76.431%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/request.js	2	11	18.18%
<!--	Total:	2	11	18.18%	-->

Files with Coverage Reduction	New Missed Lines	%
lib/request.js	2	79.62%
<!--	Total:	2		-->

Totals
Change from base Build 28:	-1.5%
Covered Lines:	322
Relevant Lines:	389

💛 - Coveralls

hisco commented 5 years ago

Thanks a lot for your collaboration,

I think that if there are pending 'streams' this change will stop these from reaching their destination as it will stop the underlying tcp connection.

Just for testing did you try to replace the original close() to destroy() - it should have the same results as your solution - LMK.

session.destroy()

MikeRalphson commented 5 years ago

Will retest and let you know.

MikeRalphson commented 5 years ago

Changing the .close to .destroy on https://github.com/hisco/http2-client/blob/4f107432d5d671e403af559168a7c2703abed0f5/lib/request.js#L314 doesn't prevent the hanging. Maybe due to the way the socket is set up (is it prior to the http2Connection?)

Is there a way to wait for connections on the socket to complete before destroying it?

hisco commented 5 years ago

Thank you for your help. The problem was specifically with h1.1 connections -the agent was not managing the connection due to as you said Maybe due to the way the socket is set up.

Unfortunately, because http1 agent manages request and is not managing sockets it was a problem to fix it as I would expect.

Therefore, I've created a pull request that actually closes the http1.1 connection, caches the result, and from now on it will be a regular http request - it seems like a better solution will require some patching or re-writing core http module.

8

LMK if the pull request solved the issue. Thanks!

hisco commented 5 years ago

Issue was solved...

Closing this PR...