Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
stream: force subscription store check as stop gap for wrapper side implementation #1717
base: master
Are you sure you want to change the base?
stream: force subscription store check as stop gap for wrapper side implementation #1717
Changes from 5 commits
3e7fe77
457cba1
dc72434
5ce010d
6ec3204
fadf919
759e225
fcf1d23
7147815
7401285
102bfc1
1e3eeda
9d7f116
ba022ec
06380e0
aca87c3
6c6fb30
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something I am missing with this?
This inclusion does not appear to make much sense to me, given that by this point all subscriptions have been set to the
Subscribed
state due the for loop above thisWhy aren't you handling the unsubscribe in the for loop above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, didn't even notice that. None of those subs were being added to the subscription store and my changes complained. I will check it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that these are being returned by
Connect()
, I think it would be nice to export these so you can help distinguish an error from connecting or an error for subscribing and act accordinglyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have this problem:
We call connect on an exchange, one pair on one asset fails to subscribe (like TRUMPEW the other day) and all connections get torn down for all asset types and subs.
I still don't agree with that.
The biggest problem I think I have is the strong coupling between connections and subs.
If a consumer wants to connect the websocket without subs, and then later call
Subscribe
manually, we both allow it, and don't support it at the same time.I think this is a wider topic, because this change doesn't make 454/460 worse, but it doubles down on the concept.
I think we need to say "Connecting is connecting, and Subscribing is separate" especially when new subs or resubs could connect new subs.
I'll continue to work on this, so I'm just highlighting for alignment on direction.
In the meantime, I'd gently request again to log the error, and return it, but not tear down all of the subs and conns for all assets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you remember what specifically caused this?
The coupling connect() with subs is not great, regarding
"Connecting is connecting, and Subscribing is separate"
though is I thinkconnect
function might be changed to handle connections that don't require subscriptions and spawn a middleware handler that performs a JIT connection based on current/incoming subscriptions which also drops connections when unsubscribing when subs is empty, that way it scales a bit better with respect to a connections max sub capacity. I don't currently have a design that could help you in that respect though.Sure, if it's easier for everyone else I will just log it and I can just modify my own trading branch to be throwing the baby out with the bathwater myself. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gloriousCode @thrasher- I pivot off these errors in my own trading branches that retries connect until it establish a clean slate or base line of full subscriptions across all trading pairs that I require. Happy to just log this out like GK suggests if we all agree and I will update tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A badly listed futures pair erroring on orderbook.
Okay. This might be the crux of our disagreement.
You should not need to disconnect and unsub everything just because one thing fails, if your goal is to ensure it eventually works.
connectionManager should be monitoring and reconnecting, and if it doesn't have requisite granularity we should improve that.
And I think subscriptionManager should manager subscriptions.
What I'm aiming for is smaller responsibilities for functions that have names (and meanings) like connect, so they're more versatile (or composable) and easier to test.
Can you throw me any other requirements you might have like "retry subs and conns until everything is working", if you have any ?
I see exactly the same situation, and also wanna know "When's everything looking good?".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good direction to have, I agree.
Pretty much just this; Personally, I am still going to rely on ensuring a clean slate of full subscriptions before proceeding. If even one subscription fails, it indicates a potential issue with the connection or the subscription logic, and I prefer to retry until everything is established correctly, or bring the instance offline and fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subscribe
is called and return errors. The refactoring you speak of gk will change that section regardless, but I get not wanting to tangle further.I propose we move subscribing for multi-connections to the end of the
connect()
function. It is a small change™️ that satisfies the following criteria:websocketroutine_manager.go
will log any errors returnedConnect()
directly to handle any errors that are returned, and resubscribe/reconnect/panic("help!")
Gist:
https://gist.github.com/gloriousCode/609d25ea1ee9b954ecbb95b505c76609
It comes with the caveat that I haven't tested things thoroughly. I tried things like separating subscribing, but that's way too large a refactor for this PR and comes with many extra considerations that I don't think are what is desired out of this PR - which is to ensure all subscriptions are subscribed to and that the caller knows it
Does this solution make people happy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's a good compromise with minimal intrusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check me please 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel right ...
That said, it's not too bad either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should, I just added it as a catch all in the event we forgot to remove it from the store when it was successful. Then it should complain. should 😬
Now you are making me think this is all completely wrong 😆. This specifically didn't catch any issues. Can you suggest a better way as a back up check? Cause I am drooling at my screen trying to figure it out 🤤.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for late reply on this.
I think I'd want see that ranging the subs and unsubs have changed State, and that store contains (or doesn't) each one.
ContainsAll
andContainsNone
or something.I really don't like checking len as a catchall, because it could false positive.
We're not locking store when we do any of this, afterall.