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.

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

Ensure websocket connection is open before sending #1078

Merged
merged 6 commits into from
Oct 10, 2017
Merged

Ensure websocket connection is open before sending #1078

merged 6 commits into from
Oct 10, 2017

Conversation

moeadham
Copy link
Contributor

to prevent unhandled error.

@coveralls
Copy link

coveralls commented Sep 26, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 85.078% when pulling dca85ec on go-faast:1.0 into e633406 on ethereum:1.0.

@coveralls
Copy link

coveralls commented Sep 26, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 85.078% when pulling dca85ec on go-faast:1.0 into e633406 on ethereum:1.0.

@moeadham
Copy link
Contributor Author

moeadham commented Oct 3, 2017

@frozeman any thoughts on this one? We are running off a fork in prod now because of this. We aren't alone if you check the issues list.

frozeman
frozeman reviewed Oct 4, 2017
View reviewed changes
packages/web3-providers-ws/src/index.js Outdated
@@ -209,6 +209,16 @@ WebsocketProvider.prototype.send = function (payload, callback) {
// try reconnect, when connection is gone
// if(!this.connection.writable)
// this.connection.connect({url: this.url});
if (this.connection.readyState !== this.connection.OPEN) {
console.trace()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the console trace here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frozeman done

@coveralls
Copy link

coveralls commented Oct 4, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 85.526% when pulling cb19365 on go-faast:1.0 into 275ece9 on ethereum:1.0.

moeadham
moeadham commented Oct 4, 2017
View reviewed changes
Copy link
Contributor Author

@moeadham moeadham left a comment

Choose a reason for hiding this comment

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

Trace removed

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 85.559% when pulling 4bffa70 on go-faast:1.0 into 275ece9 on ethereum:1.0.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 85.559% when pulling 4bffa70 on go-faast:1.0 into 275ece9 on ethereum:1.0.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 85.559% when pulling 4bffa70 on go-faast:1.0 into 275ece9 on ethereum:1.0.

@moeadham
Copy link
Contributor Author

moeadham commented Oct 4, 2017 •
edited
Loading

@frozeman This test failed due to something in shh. Should I change anything or can we move this forward?

1) web3.shh.subscribe
       should create a subscription for "syncing":
      Uncaught AssertionError: expected { Object (startingBlock, currentBlock, ...) } to deeply equal { Object (startingBlock, currentBlock, ...) }
      + expected - actual
       {
         "currentBlock": 786193
      -  "highestBlock": 786193
      +  "highestBlock": 712483
         "knownStates": 698915
         "pulledStates": 35
         "startingBlock": 786211
       }

(I don't have access to re-build)

@frozeman
Copy link
Contributor

frozeman commented Oct 6, 2017

If tests don't pass, then we can't move forward :)
Can you see if you can reproduce the failing test using npm test locally? You can trigger the rebuild, by pushing an empty commit. There are a few failing tests, can you please see why this is the case, it may have to do with your changes.

@coveralls
Copy link

coveralls commented Oct 6, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 85.559% when pulling d6a046f on go-faast:1.0 into 275ece9 on ethereum:1.0.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 6, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 85.559% when pulling d6a046f on go-faast:1.0 into 275ece9 on ethereum:1.0.

@coveralls
Copy link

coveralls commented Oct 6, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 85.559% when pulling 178eb49 on go-faast:1.0 into 275ece9 on ethereum:1.0.

@moeadham
Copy link
Contributor Author

moeadham commented Oct 6, 2017

@frozeman Sorry about that. All good now, dummy commit worked.

moeadham
moeadham commented Oct 6, 2017
View reviewed changes
Copy link
Contributor Author

@moeadham moeadham left a comment

Choose a reason for hiding this comment

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

Formatting matches project standard

@frozeman frozeman merged commit 74d612a into web3:1.0 Oct 10, 2017
@frozeman
Copy link
Contributor

Thanks!

nachomazzara pushed a commit to nachomazzara/web3.js that referenced this pull request Jun 4, 2020
@moeadham @nachomazzara
Ensure websocket connection is open before sending (web3#1078)
9dc9d1b 
* Ensure websocket connection is open before sending to prevent unhandled error.

* [console] remove trace on ws error

* Trigger tests rebuild

* [formatting] add semicolons;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frozeman frozeman frozeman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@moeadham @coveralls @frozeman