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

Problem: subscription refactoring not merged #375

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Nov 14, 2023

Description

Merge the feature into develop branch.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

* Problem: internal websocket connection is heavy and unstable

Solution:
- use local node client directly.

* ignore duplicate subscriptions, it's possible in concurrency

* bigger start timeout

* handle error

* fix lint

* fix unit test

* rename

* fix test

* remove test

* add buffer
* implement stream

* rpc stream

* websocket use stream

* filter apis use stream

* cleanup

* move modules

* cleanup

* fix cond race

* fix lint

* nit

* fix build

* fix lint

* err return

* fix empty logs

* fix filter initial offset

* fix filter

* fix filter

* fix test
@yihuang yihuang requested a review from mmsqe November 14, 2023 02:40
}, api.logger, "closing websocket peer sub")
try(func() {
if err != websocket.ErrCloseSent {
_ = wsConn.Close()

Check warning

Code scanning / gosec

Returned error is not propagated up the stack. Warning

Returned error is not propagated up the stack.
if err != nil {
try(func() {
if err != websocket.ErrCloseSent {
_ = wsConn.Close()

Check warning

Code scanning / gosec

Returned error is not propagated up the stack. Warning

Returned error is not propagated up the stack.
api.logger.Debug("dropping NewHeads WebSocket subscription", "subscription-id", subID, "error", err.Error())
try(func() {
if err != websocket.ErrCloseSent {
_ = wsConn.Close()

Check warning

Code scanning / gosec

Returned error is not propagated up the stack. Warning

Returned error is not propagated up the stack.
Copy link
Collaborator

@mmsqe mmsqe left a comment

Choose a reason for hiding this comment

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

do we need an improvement change log?

@yihuang
Copy link
Collaborator Author

yihuang commented Nov 14, 2023

do we need an improvement change log?

sure, added.

@yihuang yihuang merged commit 8b1ec23 into develop Nov 14, 2023
24 of 27 checks passed
yihuang added a commit that referenced this pull request Nov 14, 2023
* Problem: internal websocket connection is heavy and unstable (#373)

* Problem: internal websocket connection is heavy and unstable

Solution:
- use local node client directly.

* ignore duplicate subscriptions, it's possible in concurrency

* bigger start timeout

* handle error

* fix lint

* fix unit test

* rename

* fix test

* remove test

* add buffer

* Problem: channels are not efficient for broadcasting scenario (#374)

* implement stream

* rpc stream

* websocket use stream

* filter apis use stream

* cleanup

* move modules

* cleanup

* fix cond race

* fix lint

* nit

* fix build

* fix lint

* err return

* fix empty logs

* fix filter initial offset

* fix filter

* fix filter

* fix test

* changelog

* add comments
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.

2 participants