lntest: set ReadHeaderTimeout for http client

[bshramin]

Change Description

Add ReadHeaderTimeout for HTTP client in lntests.
Fixes #7232
The same code can be found here: https://github.com/lightningnetwork/lnd/blob/master/tls_manager.go#L424

Steps to Test

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

[ellemouton]

Thanks for the PR @bshramin :)

I dont think this fixes #7232 though - that issue is suggesting that we should make the timeout configurable.

[bshramin]

Thanks for the PR @bshramin :)

I dont think this fixes #7232 though - that issue is suggesting that we should make the timeout configurable.

You're right, but it's still an improvement. I do need some guidance for making it configurable.

[Chinwendu20]

My understanding is that there should be a global variable for the ReadHeaderTimeout and this variable should be included as an arg for every occurrence of http.Server. In that way the timeout is configurable. IMO, I think the PR is a good start, just that it does not cover all the instances and the variable is not global per see? Maybe creating another package in the internal folder, where the variable would live would suffice.

[ellemouton]

To make it configurable, you need to add a variable to the main LND Config struct. The value can then be pulled from that struct (as is done for the Addr: cfg.Profile line two lines above your change

[bshramin]

al variable for the ReadHeaderTimeout and this variable should be included as an arg for every occurrence of http.Server. In that way the timeout is configurable. IMO, I think the PR is a good start, just that it does not cover all the instances and the variable is not global per see? Maybe creating another package in the internal folder, where the variable would live would suffice.

Will work on it soon.

[bshramin]

@ellemouton Does this look better?

[ellemouton]

@bshramin - looks better but a few things still missing:

1. we should use the config value for all http.Server instances too(for example, the TlsManager one)
2. with the current version of the PR, it will set the timeout to zero. We still need to use the 5 seconds as a default

[bshramin]

@bshramin - looks better but a few things still missing:

1. we should use the config value for all http.Server instances too(for example, the TlsManager one)
2. with the current version of the PR, it will set the timeout to zero. We still need to use the 5 seconds as a default

Thank you, Elle. How are we looking now?

[yyforyongyu]

Thanks for the PR! The commit messages don't have the right format, for reference pls check this link.

[bshramin]

@yyforyongyu Thank you for the comments. Is it better now?

[yyforyongyu]

Very close! Could you take a look at the CI? It gives you instructions on what needs to be fixed,

• need to update the sample config
• need to add release notes(0.18.0)

The itests are flakes which can be ignored atm except this one,

--- FAIL: TestLightningNetworkDaemon/tranche02/70-of-135/bitcoind/REST_API/websocket_bi-directional_subscription (59.05s)

Not sure if it's related to this PR.

[bshramin]

Very close! Could you take a look at the CI? It gives you instructions on what needs to be fixed,

• need to update the sample config
• need to add release notes(0.18.0)

The itests are flakes which can be ignored atm except this one,

--- FAIL: TestLightningNetworkDaemon/tranche02/70-of-135/bitcoind/REST_API/websocket_bi-directional_subscription (59.05s)

Not sure if it's related to this PR.

Thank you for the detailed explanations. Is it better now?

[yyforyongyu]

LGTM🎉

[guggero]

Thanks for the update! Very very close, just three small comments.

[guggero]

LGTM, thanks 🎉