Skip to content

refactor(rpc): allocate PingPongLatencyTimer on start#2804

Merged
melekes merged 1 commit intocometbft:mainfrom
firelizzard18:refactor-wsclient-2
Apr 15, 2024
Merged

refactor(rpc): allocate PingPongLatencyTimer on start#2804
melekes merged 1 commit intocometbft:mainfrom
firelizzard18:refactor-wsclient-2

Conversation

@firelizzard18
Copy link
Contributor

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 to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@firelizzard18 firelizzard18 requested a review from a team as a code owner April 12, 2024 21:03
@firelizzard18 firelizzard18 requested a review from a team April 12, 2024 21:03
@melekes melekes added backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x labels Apr 14, 2024
@cason
Copy link

cason commented Apr 15, 2024

Another example for #2041.

Copy link

@cason cason left a comment

Choose a reason for hiding this comment

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

+1

@melekes melekes added this pull request to the merge queue Apr 15, 2024
Merged via the queue into cometbft:main with commit 312d1d4 Apr 15, 2024
mergify bot pushed a commit that referenced this pull request Apr 15, 2024
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 bot pushed a commit that referenced this pull request Apr 15, 2024
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)
melekes pushed a commit that referenced this pull request Apr 15, 2024
#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 pull request Apr 15, 2024
#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>
@firelizzard18 firelizzard18 deleted the refactor-wsclient-2 branch April 15, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP client leaks PingPongLatencyTimer

3 participants