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

Fix bug in event monitor where subscriptions would not be terminated properly on WebSocket error #290

Merged
merged 5 commits into from
Oct 8, 2020

Conversation

romac
Copy link
Member

@romac romac commented Oct 6, 2020

Closes: #293

Description

This PR fixes a potential bug where subscriptions would not be terminated properly if the WebSocket client encountered an error because

a) we were emptying the subscriptions vector when passing it to select_all, and
b) we were not closing the client explicitly

To test this PR:

Use the following configuration file:

title = "IBC Relayer Config Example"

[global]
timeout = "10s"
strategy = "naive"

[[chains]]
  id = "chain_A"
  rpc_addr = "localhost:26657"
  account_prefix = "cosmos"
  key_name = "testkey"
  store_prefix = "ibc"
  client_ids = ["clA1", "clA2"]
  gas = 200000
  gas_adjustement = 1.3
  gas_price = "0.025stake"
  trusting_period = "336h"

[[connections]]

In one terminal, spawn a Tendermint node running the KV Store app:

docker run -it --rm -v "/tmp/tendermint:/tendermint" tendermint/tendermint init
docker run -it --rm -v "/tmp/tendermint:/tendermint" -p 26657:26657 tendermint/tendermint node --proxy_app=kvstore

In another terminal, from the relayer-cli folder, run the listen command of the relayer:

$ cargo run -- -c path/to/config/file.toml listen

Expected output:

Oct 06 18:01:35.126  INFO relayer_cli::commands::listen: spawning event monitor for chain.id=chain_A
Oct 06 18:01:35.159  INFO relayer_cli::commands::listen: spawning main event handler
Oct 06 18:01:35.160  INFO relayer::event_monitor: running listener for chain.id=chain_A
Oct 06 18:01:35.160  INFO relayer::event_handler: running IBC Event Handler
Oct 06 18:01:35.343  INFO relayer::event_handler: Chain chain_A pushed {"NewBlock":{"height":"322"}}
Oct 06 18:01:35.863  INFO relayer::event_handler: Chain chain_A pushed {"NewBlock":{"height":"323"}}

…

For contributor use:

  • Unit tests written
  • Added test to CI if applicable
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer

@romac romac changed the title Fix subscriptions Fix event monitor after tendermint-rs update Oct 6, 2020
@romac romac changed the title Fix event monitor after tendermint-rs update Fix bug in event monitor where subscriptions would not be terminated properly on WebSocket error Oct 6, 2020
@romac romac marked this pull request as ready for review October 6, 2020 16:04
@romac romac requested a review from ancazamfir as a code owner October 6, 2020 16:04
@romac romac requested a review from thanethomson October 6, 2020 16:08
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #290 into master will increase coverage by 23.4%.
The diff coverage is 62.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #290      +/-   ##
=========================================
+ Coverage    13.6%   37.1%   +23.4%     
=========================================
  Files          69     120      +51     
  Lines        3752    7743    +3991     
  Branches     1374    2734    +1360     
=========================================
+ Hits          513    2875    +2362     
- Misses       2618    4561    +1943     
+ Partials      621     307     -314     
Impacted Files Coverage Δ
modules/src/events.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/events.rs 0.0% <ø> (ø)
modules/src/ics02_client/raw.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/context.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/error.rs 16.6% <0.0%> (-16.7%) ⬇️
modules/src/ics04_channel/error.rs 75.0% <ø> (+50.0%) ⬆️
modules/src/ics04_channel/packet.rs 0.0% <0.0%> (ø)
modules/src/ics07_tendermint/client_def.rs 0.0% <0.0%> (ø)
modules/src/ics07_tendermint/error.rs 75.0% <ø> (+75.0%) ⬆️
modules/src/ics18_relayer/error.rs 0.0% <0.0%> (ø)
... and 218 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5024717...45cc9bc. Read the comment docs.

@romac romac merged commit 6fe7366 into master Oct 8, 2020
@romac romac deleted the romac/fix-subscriptions branch October 8, 2020 16:20
@adizere
Copy link
Member

adizere commented Oct 8, 2020

FYI, cargo build on master fails following the work in this PR:

   Compiling relayer v0.0.3 (/Users/adi/Hammers/ibc-rs/relayer)
error[E0432]: unresolved imports `tendermint_rpc::query`, `tendermint_rpc::query`
 --> relayer/src/event_monitor.rs:4:5
  |
4 |     query::EventType, query::Query, Subscription, SubscriptionClient, WebSocketClient,
  |     ^^^^^             ^^^^^ could not find `query` in `tendermint_rpc`
  |     |
  |     could not find `query` in `tendermint_rpc`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0432`.
error: could not compile `relayer`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed

LE: had to do a cargo update and cleanup, then build worked!

hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…properly on WebSocket error (informalsystems#290)

* Store the combined stream of all subscriptions within the EventMonitor

* Explicitly close the WebSocket client when this one fails to ensure that subscriptions are terminated

* Fix event type in Tx subscription

* Fix build for latest tendermint-rs master

* Formatting

Co-authored-by: Thane Thomson <connect@thanethomson.com>
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.

master build fails
4 participants