Skip to content

Add keepAlive agent#57

Merged
Joozty merged 1 commit intomasterfrom
agent
Jun 15, 2020
Merged

Add keepAlive agent#57
Joozty merged 1 commit intomasterfrom
agent

Conversation

@Joozty
Copy link
Contributor

@Joozty Joozty commented Jun 11, 2020

No description provided.

@Joozty Joozty changed the base branch from dev to master June 11, 2020 14:22
@Joozty Joozty self-assigned this Jun 11, 2020
@Joozty Joozty marked this pull request as ready for review June 15, 2020 07:37
@Joozty Joozty requested a review from viktor-yakubiv June 15, 2020 07:38
@viktor-yakubiv
Copy link

What is rationable behind?

@Joozty
Copy link
Contributor Author

Joozty commented Jun 15, 2020

The Reader performs a lot of API calls for article metadata. This optimisation should improve response time because this keeps sockets open for next calls, i.e. there is no need to TCP handshake all over again.

Copy link

@viktor-yakubiv viktor-yakubiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why could not you do this just passing Keep-Alive: 'timeout=60000, max=1000 header to axios?

@Joozty
Copy link
Contributor Author

Joozty commented Jun 15, 2020

AgentKeepAlive library allows to have pool of free sockets and has more parameters to tweak I would say.

@Joozty Joozty merged commit f39bed2 into master Jun 15, 2020
@Joozty Joozty deleted the agent branch June 15, 2020 08:21
@viktor-yakubiv
Copy link

I don't see any other option for this library to work in the browser where you cannot have any control of the connection pool apart having the header I wrote about.

Also, I am not sure it's possible to reuse XMLHttpRequest instance and pretty sure it's not possible to reuse Fetch API promise.

@Joozty
Copy link
Contributor Author

Joozty commented Jun 15, 2020

This is only for node. I tested it in browser and API worked fine. I guess Axios handles it somehow and disables agent for browser. We don't have this issue in the browser so it's fine as-is for now.

@viktor-yakubiv
Copy link

Thank you for the explanation

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants