Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeout errors with clever deploy (the git push part) #640

Closed
hsablonniere opened this issue Dec 1, 2023 · 0 comments · Fixed by #641 or #661
Closed

Timeout errors with clever deploy (the git push part) #640

hsablonniere opened this issue Dec 1, 2023 · 0 comments · Fixed by #641 or #661
Labels

Comments

@hsablonniere
Copy link
Member

Thanks to @JulienBrgs, we identified a very nasty bug:

🤯 The problem

Sometimes clever deploy fails with a timeout during the git push phase.

  • It happens with Node.js 20 but not with Node.js 18.
  • We thought it only happened with big payloads (when lots of commits and data needs to be pushed) but it's not really clear.

🧐 The details

Here's what happens:

Why does it work with Node.js 18 but not with Node.js 20?

Our "timeout bug" is caused by this Node.js update but why would isomorphic-git (via simple-get) get a timeout error if it's genuinely pushing data with an HTTP POST?

I wrote a reduced test case with a basic HTTP server and some POST calls using simple-get to confirm the origin:

  • With Node.js 18, you can trigger the timeout error by using an agent with the same settings as Node.js 20 (keepAlive: true and timeout: 5000).
  • With Node.js 20, you can prevent the timeout error by using an agent without the timeout option.
    • The keepAlive: true is not the cause of the timeout.
    • You could also use agent: false with http.request, but it would prevent the different requests to reuse the same HTTP socket/connection.

Then I tried with an agent configured with a timeout of 5 seconds and different server behaviours to understand what situations trigger the timeout error.

  • If the server hasn't sent the response status yet, but refuses to receive any data from the request body for more than 5 secondes, the client will get a timeout error.
  • If the server has already sent the response status, but refuses to send a response body and finish the response, it will trigger a timeout on the client.

After discussing this with my colleagues, we think our git server sometimes wait a bit too much before accepting the request body (to validate the auth or something).
It's rare but it can happen in some situations so we need to remove this 5 seconds timeout option introduced in Node.js 19.

💡 The solutions

  • (1) Ask simple-get to use a default agent without a timeout
    • We'll have to do a PR, wait for a merge, a release and then the same for isomorphic-git as it's a transitive dep
    • This may not be the behaviour other users want
  • (2) Ask isomorphic-git to pass an agent without a timeout to simple-get
    • We'll have to do a PR, wait for a merge and a release
    • This may not be the behaviour other users want
  • (3) Modify the http.globalAgent temporarily before/after the git push
    • This seems like the simplest solution but it looks a bit like a hack
  • (4) Create an http implementation for isomorphic-git that does not have this timeout setting

➡️ Let's go for solution (4).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant