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

Contract Registry - trait and centralized implementation #182

Merged
merged 1 commit into from
May 4, 2018

Conversation

willscott
Copy link
Contributor

Parallel to the entity / node registry

  • does a contract own itself? Should it be signed / owned by an entity?

@willscott willscott added p:1 Priority: core feature c:registry Category: entity/node/runtime registry service labels May 3, 2018
@willscott willscott requested a review from Yawning May 3, 2018 20:10
@kostko
Copy link
Member

kostko commented May 4, 2018

does a contract own itself? Should it be signed / owned by an entity?

What do you mean by itself? A contract has a public key? Who has the private part, the key manager?

let mut inner = inner.lock().unwrap();
inner.contracts.insert(contract.id, contract.clone());

inner.notify(&contract);
Copy link
Member

@kostko kostko May 4, 2018

Choose a reason for hiding this comment

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

This may lead to a deadlock in case the gRPC executor is used (which we are currently using). The issue is that with the gRPC executor, as an optimization, notifying a task may poll the task immediately if it is running on the same thread. In this concrete case, when you call unbounded_send above, it will cause the receiver task (the one that is running the get_contracts futrue) to get notified and the gRPC executor will immediately poll the task (e.g., before unbounded_send returns). If that task then polls any registry future that takes the same lock, this will cause a deadlock because the lock is still held and mutexes in Rust are not recursive.

For this reason I had to complicate the dummy consensus backend and introduce more granular locking in #172 (see the dummy consensus backend changes). Thinking about it now, the most correct thing would be to avoid taking any lock while calling unbounded_send, but this way you cannot remove subscribers. Perhaps a RwLock would be the correct primitve to use here.

Copy link
Member

Choose a reason for hiding this comment

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

I've implemented a general structure for handling stream subscribers in #183.

Just refactor your code to release the inner lock before calling its notify (e.g. make locks more granular so that there is a mutex covering inner.contracts but not inner.subscribers and that inner is only in an Arc without a Mutex).

@willscott
Copy link
Contributor Author

willscott commented May 4, 2018 via email

@kostko
Copy link
Member

kostko commented May 4, 2018 via email

@willscott
Copy link
Contributor Author

willscott commented May 4, 2018 via email


pub use backend::*;
pub use contract_backend::*;
Copy link
Member

Choose a reason for hiding this comment

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

If this is now contract_backend, could we name the other one entity_backend? And the same for services. Otherwise it looks a bit confusing.

@willscott
Copy link
Contributor Author

for now, we'll have the contract id pubkey not have explicit relationship with an 'entity' - since there are lifetime issues (entities can be deregistered, unlike contracts).

cargo fmt

Implement StreamSubscribers per #183

rename registry 'backend' to 'entity' throughout

fmt
@willscott willscott force-pushed the willscott/feature/contract-registry branch from 8569489 to 12acb2c Compare May 4, 2018 22:17
@willscott willscott merged commit 318b879 into master May 4, 2018
@willscott willscott deleted the willscott/feature/contract-registry branch May 4, 2018 22:33
@willscott willscott mentioned this pull request May 10, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:registry Category: entity/node/runtime registry service p:1 Priority: core feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants