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

Websockets - check for subscriptions before proceeding #1906

Conversation

@guilhermelawless
Copy link
Contributor

commented Apr 14, 2019

If the websocket server is active via config but there are no active subscriptions, currently the observer will still create and try to broadcast the block confirmation message.

A simple check would have been to go through all sessions and check if any topics are subscribed. However, since this would be done every time a new block arrives, I have added a map topic -> subscription count in the listener to do this lookup in constant time. I'm open to suggestions as I am not sure what the right pattern would be here.

Small test added for this as well.

@cryptocode
Copy link
Collaborator

left a comment

Broadcasting is essentially free if there are no sessions. For the case where there are sessions: if you open up websockets to potentially malicious clients, they'd probably subscribe since that's more severe than connecting and not subscribing.

I can see the need for something like this if we get a lot more topics. Is the added complexity worth it for the current confirmation topic / do you think it solves a problem now? The only scenario I can think of is applications connecting but only occasionally listening to confirmations, but not sure what the use case would be.

Show resolved Hide resolved nano/node/websocket.cpp Outdated
@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@cryptocode it was definitely with the case of different subscriptions in mind. In that case this becomes crucial.

Most likely anyone with websocket enabled will be connecting to it, but maybe not always? I will have something that I will connect only occasionally to gather information about votes/blocks, so it wouldn't make sense to always go through the message building process if that is not currently connected.

The added complexity is to avoid the following for every block:

if (this->block_arrival.recent (block_a->hash ()))
{
	std::string subtype;
	if (is_state_send_a)
	{
		subtype = "send";
	}
	else if (block_a->type () == nano::block_type::state)
	{
		if (block_a->link ().is_zero ())
		{
			subtype = "change";
		}
		else if (amount_a == 0 && !this->ledger.epoch_link.is_zero () && this->ledger.is_epoch_link (block_a->link ()))
		{
			subtype = "epoch";
		}
		else
		{
			subtype = "receive";
		}
	}
	nano::websocket::message_builder builder;
	auto msg (builder.block_confirmed (block_a, account_a, amount_a, subtype));
	this->websocket_server->broadcast (msg);
}

If we have other subscription types in the future, their processing might be more resource heavy so it should be avoided.

Show resolved Hide resolved nano/node/websocket.hpp Outdated
@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

@guilhermelawless makes sense, I think we should add this.

@cryptocode cryptocode added this to the V19.0 milestone Apr 14, 2019

@cryptocode cryptocode added this to During RC in V19 Apr 14, 2019

std::vector<std::weak_ptr<session>> sessions;
std::unordered_map<topic, std::size_t> topic_subscription_count;

This comment has been minimized.

Copy link
@clemahieu

clemahieu Apr 14, 2019

Collaborator

Since the number of topics is small, this could be an array indexed by the enum value.

This comment has been minimized.

Copy link
@cryptocode

cryptocode Apr 14, 2019

Collaborator

Doing so would also fix the CI build error (missing hash specialization for the enum class I think, required on some compilers)

This comment has been minimized.

Copy link
@guilhermelawless

guilhermelawless Apr 14, 2019

Author Contributor

Good idea, also could change to atomic size_t and thus remove the mutex. Getting the length of the enum is cumbersome.

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

You should be able to size an enum by creating a value at the end called something like topic_count which doesn’t represent an actual topic.

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Ok that's how I did it in 9e8471f

@cryptocode cryptocode merged commit ac89b64 into nanocurrency:master Apr 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cryptocode cryptocode moved this from During RC to CP3 (2019-04-10) in V19 Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.