8000 HTTP client leaks PingPongLatencyTimer · Issue #2771 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

HTTP client leaks PingPongLatencyTimer #2771

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.

Sign up for GitHub

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

Jump to bottom
Closed
firelizzard18 opened this issue Apr 10, 2024 · 0 comments · Fixed by #2792 or #2804
Closed

HTTP client leaks PingPongLatencyTimer #2771

firelizzard18 opened this issue Apr 10, 2024 · 0 comments · Fixed by #2792 or #2804
Labels
bug Something isn't working

Comments

@firelizzard18
Copy link
Contributor
firelizzard18 commented Apr 10, 2024 •
edited
Loading

Bug Report

What happened?

rpc/jsonrpc/client.WSClient starts a github.com/rcrowley/go-metrics.Timer but never closes it, thus rpc/client/http.HTTP leaks.

What did you expect to happen?

Either:

  • Some method on HTTP that releases the timer; or
  • The timer is released when HTTP.Stop is called and not allocated in the first place unless HTTP.Start is called.

How to reproduce it

Call rpc/client/http.NewClient.

Anything else we need to know

Besides the field definition and initialization, PingPongLatencyTimer is only referenced once: c.PingPongLatencyTimer.UpdateSince(t). There are no calls to c.PingPongLatencyTimer.Stop(). More significantly, it isn't used by anything. PingPongLatencyTimer appears to be dead code. It could be completely removed without affecting Tendermint.
@firelizzard18 firelizzard18 added bug Something isn't working needs-triage This issue/PR has not yet been triaged by the team. labels Apr 10, 2024
@hoanguyenkh hoanguyenkh mentioned this issue Apr 12, 2024
Merged
4 tasks
github-merge-queue bot pushed a commit that referenced this issue Apr 12, 2024
@hoanguyenkh
refactor(rpc): miss stop PingPongLatencyTimer (#2792)
3d736a3 
Closes #2771
<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
mergify bot pushed a commit that referenced this issue Apr 12, 2024
@hoanguyenkh @mergify
refactor(rpc): miss stop PingPongLatencyTimer (#2792)
5ba8082 
Closes #2771
<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit 3d736a3)
mergify bot pushed a commit that referenced this issue Apr 12, 2024
@hoanguyenkh @mergify
refactor(rpc): miss stop PingPongLatencyTimer (#2792)
62c5a01 
Closes #2771
<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit 3d736a3)
This was referenced Apr 12, 2024
Merged
Merged
melekes pushed a commit that referenced this issue Apr 12, 2024
@mergify @hoanguyenkh
refactor(rpc): miss stop PingPongLatencyTimer (backport #2792) (#2794)
3872582 
Closes #2771


---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2792 done by
[Mergify](https://mergify.com).

Co-authored-by: Khanh Hoa <49144992+hoanguyenkh@users.noreply.github.com>
melekes pushed a commit that referenced this issue Apr 12, 2024
@mergify @hoanguyenkh
refactor(rpc): miss stop PingPongLatencyTimer (backport #2792) (#2795)
e5cf488 
Closes #2771


---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2792 done by
[Mergify](https://mergify.com).

Co-authored-by: Khanh Hoa <49144992+hoanguyenkh@users.noreply.github.com>
@firelizzard18 firelizzard18 mentioned this issue Apr 12, 2024
Merged
4 tasks
firelizzard18 added a commit to AccumulateNetwork/accumulate that referenced this issue Apr 14, 2024
@firelizzard18
Replace Tendermint's HTTP client [#3590]
d32fd0a 
Closes #3590. Tendermint's HTTP client leaks. Specifically the WebSocket/event support has a supervisor routine that is leaked. So stole Tendermint's HTTP client sans WebSockets.

This is a workaround for cometbft/cometbft#2771.

Changelog: fix
github-merge-queue bot pushed a commit that referenced this issue Apr 15, 2024
@firelizzard18
refactor(rpc): allocate PingPongLatencyTimer on start (#2804)
312d1d4 
Followup to #2792 which closed #2771.

#2792 does not handle the case where Start is never called. If Start is
not called, Stop returns an error, thus with #2792's implementation the
only way to ensure that PingPongLatencyTimer is cleaned up is to call
Start and Stop, even when not using any of the features provided by
Start (i.e. events).

This PR moves initialization of PingPongLatencyTimer into OnStart so
that it is only initialized if it is going to be used. This PR also
moves cleanup of PingPongLatencyTimer into readRoutine's defer statement
to align it with other cleanup (i.e. closing the connection).

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
mergify bot pushed a commit that referenced this issue Apr 15, 2024
@firelizzard18 @mergify
refactor(rpc): allocate PingPongLatencyTimer on start (#2804)
4ac27bf 
Followup to #2792 which closed #2771.

#2792 does not handle the case where Start is never called. If Start is
not called, Stop returns an error, thus with #2792's implementation the
only way to ensure that PingPongLatencyTimer is cleaned up is to call
Start and Stop, even when not using any of the features provided by
Start (i.e. events).

This PR moves initialization of PingPongLatencyTimer into OnStart so
that it is only initialized if it is going to be used. This PR also
moves cleanup of PingPongLatencyTimer into readRoutine's defer statement
to align it with other cleanup (i.e. closing the connection).

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit 312d1d4)
@mergify mergify bot mentioned this issue Apr 15, 2024
Merged
4 tasks
mergify bot pushed a commit that referenced this issue Apr 15, 2024
@firelizzard18 @mergify
refactor(rpc): allocate PingPongLatencyTimer on start (#2804)
0103516 
Followup to #2792 which closed #2771.

#2792 does not handle the case where Start is never called. If Start is
not called, Stop returns an error, thus with #2792's implementation the
only way to ensure that PingPongLatencyTimer is cleaned up is to call
Start and Stop, even when not using any of the features provided by
Start (i.e. events).

This PR moves initialization of PingPongLatencyTimer into OnStart so
that it is only initialized if it is going to be used. This PR also
moves cleanup of PingPongLatencyTimer into readRoutine's defer statement
to align it with other cleanup (i.e. closing the connection).

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit 312d1d4)
@mergify mergify bot mentioned this issue Apr 15, 8000 2024
Merged
4 tasks
melekes pushed a commit that referenced this issue Apr 15, 2024
@mergify @firelizzard18
refactor(rpc): allocate PingPongLatencyTimer on start (backport #2804) (
365d4e4 
#2813)

Followup to #2792 which closed #2771.

#2792 does not handle the case where Start is never called. If Start is
not called, Stop returns an error, thus with #2792's implementation the
only way to ensure that PingPongLatencyTimer is cleaned up is to call
Start and Stop, even when not using any of the features provided by
Start (i.e. events).

This PR moves initialization of PingPongLatencyTimer into OnStart so
that it is only initialized if it is going to be used. This PR also
moves cleanup of PingPongLatencyTimer into readRoutine's defer statement
to align it with other cleanup (i.e. closing the connection).

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2804 done by
[Mergify](https://mergify.com).

Co-authored-by: Ethan Reesor <firelizzard@gmail.com>
melekes added a commit that referenced this issue Apr 15, 2024
@mergify @firelizzard18 @melekes
refactor(rpc): allocate PingPongLatencyTimer on start (backport #2804) (
9875622 
#2812)

Followup to #2792 which closed #2771.

#2792 does not handle the case where Start is never called. If Start is
not called, Stop returns an error, thus with #2792's implementation the
only way to ensure that PingPongLatencyTimer is cleaned up is to call
Start and Stop, even when not using any of the features provided by
Start (i.e. events).

This PR moves initialization of PingPongLatencyTimer into OnStart so
that it is only initialized if it is going to be used. This PR also
moves cleanup of PingPongLatencyTimer into readRoutine's defer statement
to align it with other cleanup (i.e. closing the connection).

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2804 done by
[Mergify](https://mergify.com).

Co-authored-by: Ethan Reesor <firelizzard@gmail.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@sergio-mena sergio-mena removed the needs-triage This issue/PR has not yet been triaged by the team. label Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@firelizzard18 @sergio-mena
0