Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

fix(parity-clib-example): remove websocket #10172

Closed
wants to merge 1 commit into from

Conversation

niklasad1
Copy link
Collaborator

@niklasad1 niklasad1 commented Jan 12, 2019

I suspect there is something wrong with the WebSocket subscription in the light client so let's remove it for now in order not to deny well-formed PRs!

/cc @tomusdrw @ngotchac

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M1-ci 🙉 Continuous integration. labels Jan 12, 2019
@tomusdrw
Copy link
Collaborator

@niklasad1 you can subscribe to eth_subscribe(newHeads) @mattrutherford recently changed this so it's emitted even when syncing (see #9987)

@niklasad1
Copy link
Collaborator Author

niklasad1 commented Jan 13, 2019

Thanks @tomusdrw

I found out this yesterday too when I tested it locally,

I was wrong the WebSocket works without being fully synced tested locally now but it seems to some problem with ChainNotify in the light-client sometimes it works and sometimes not. I think I had some problem with it before but has worked locally for me for a while now

Thus, I need to debug if there is something weird with the ChainNotify for the light client (I have not confirmed it yet) and should probably be disabled for the CI to prevent other PRs to fail randomly.

* I suspect there is something wrong with the WebSocket subscription in the light client so let's remove it for now
in order not to deny well-formed PRs!
@niklasad1 niklasad1 force-pushed the parity-clib/cpp-example-no-websockets branch from efc6c97 to 1b28774 Compare January 13, 2019 10:01
@5chdn 5chdn added this to the 2.4 milestone Jan 13, 2019
Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

LGTM, maybe create an issue to run those tests somewhere else in the near-future so we don't forget?

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 16, 2019
@tomaka
Copy link
Contributor

tomaka commented Feb 3, 2019

I can also be the C++ example or C bindings that have an undefined behaviour, as opposed to the Rust code having a bug.

@niklasad1
Copy link
Collaborator Author

Closing until the error-cause is confirmed which could be UB as @tomaka stated!

@niklasad1 niklasad1 closed this Feb 3, 2019
@tomaka tomaka deleted the parity-clib/cpp-example-no-websockets branch February 4, 2019 00:05
@tomaka tomaka restored the parity-clib/cpp-example-no-websockets branch February 4, 2019 00:06
@5chdn 5chdn removed the A8-looksgood 🦄 Pull request is reviewed well. label Feb 7, 2019
@soc1c soc1c deleted the parity-clib/cpp-example-no-websockets branch March 20, 2019 11:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
M1-ci 🙉 Continuous integration. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants