Dapp refresh #5752

CraigglesO · 2017-06-02T17:55:35Z

Closes: #4194

If adding or removing a dapp, this will update the list upon:

removing a dapp from the dapps folder
linking a dapp
a 404.

If you want to quickly test it:
git clone https://github.com/paritytech/skeleton mydapp
git cd mydapp

ln -s $PWD/dist $HOME/Library/Application\ Support/io.parity.ethereum/dapps/mydapp
(If you are using windows go to the tutorial... :P)

Click refresh

and to test removal:
cd ~/Library/Application Support/io.parity.ethereum/dapps
or cd /Users/$USER/Library/Application Support/io.parity.ethereum/dapps

rm -rf mydapp

rphmeier · 2017-06-05T19:01:56Z

dapps/src/endpoint.rs

@@ -39,7 +40,7 @@ pub struct EndpointInfo {
 	pub icon_url: String,
 }

-pub type Endpoints = Arc<BTreeMap<String, Box<Endpoint>>>;
+pub type Endpoints = Arc<RwLock<BTreeMap<String, Box<Endpoint>>>>;


why add RwLock if it's only read through? doesn't seem to need any lock at all

Well, the end goal is to push to the BTreeMap new dapp entries as a js developer adds. Currently you have to restart parity to get the new dapp.

tomusdrw

Few grumbles.

tomusdrw · 2017-07-05T12:45:19Z

dapps/Cargo.toml

@@ -14,6 +14,7 @@ futures = "0.1"
 linked-hash-map = "0.3"
 log = "0.3"
 parity-dapps-glue = "1.7"
+parking_lot = "0.4"


RwLock is re-export in util.

tomusdrw · 2017-07-05T12:47:19Z

dapps/src/apps/mod.rs

+	let mut v = Vec::new();
+	// accumulate the dead dapps
+	for (k, _) in pages.read().iter() {
+		if k == "ui" || k == "proxy" || k == WEB_PATH {


That's bad, we should export a list of local filesystem dapps in all_endpoints (for instance return Endpoints and Vec<String>)

tomusdrw · 2017-07-05T12:50:44Z

dapps/src/apps/mod.rs

+		if k == "ui" || k == "proxy" || k == WEB_PATH {
+			continue;
+		} else if new_pages.contains_key(k) != true {
+			v.push(k.clone());


If you keep the list of local endpoints the code here can be greately simplified (just let to_remove = old_local.iter().partition(|x| !new_local.contains_key(x)))

tomusdrw · 2017-07-05T12:52:08Z

dapps/src/apps/mod.rs

+	// new dapps to be added
+	for (k, v) in new_pages {
+		if pages.read().contains_key(&k) != true {
+			pages.write().insert(k, v);


You should acquire single lock at the top of the function instead of using read()/write() on every iteration.

I must have bad luck, but the reason I used a vector and the reason I use short term locks, (and maybe this is a unique issue to parking_lot) is that if you have a read lock, the write lock will wait indefinitely for the read lock to finish before creating it's own lock. Has no one else run into this?

You should only take one write() lock at the top (you will be able to read from it as well).

it's actually a safety feature to deadlock instead of aliasing writeable pointers

Ok, so:
a) if the function is not too complex, it's ok to have a write lock for the time of the function
b) with a write, it can also handle reads (unless handling mutable and immutable in the same instance [.iter()])

tomusdrw · 2017-07-05T12:52:50Z

dapps/src/apps/mod.rs

+	Arc::new(RwLock::new(pages))
+}
+
+pub fn refresh_local_endpoints(


IMHO this function should be part of Endpoints

tomusdrw · 2017-07-05T12:54:31Z

dapps/src/apps/ui.rs

@@ -22,6 +22,7 @@ use endpoint::{Endpoint, Handler, EndpointPath};
 use {address, handlers};

 /// Redirection to UI server.
+#[derive(Clone)]


Why is it needed?

Didn't clean it after playing with the code -_-

tomusdrw · 2017-07-05T13:03:16Z

dapps/src/router.rs

@@ -61,14 +62,16 @@ impl http::RequestMiddleware for Router {
 		let is_head_request = *req.method() == hyper::Method::Head;

 		trace!(target: "dapps", "Routing request to {:?}. Details: {:?}", url, req);
+		let refresh = self.endpoints.clone();


Why is it cloned here?

tomusdrw · 2017-07-05T13:04:51Z

dapps/src/router.rs

@@ -109,6 +112,7 @@ impl http::RequestMiddleware for Router {
 				=>
 			{
 				trace!(target: "dapps", "Resolving to 404.");
+				refresh.unwrap().refresh_local_dapps();


self.endpoints can be empty so you should unwrap.
It should be just self.endpoints.as_ref().map(|endpoints| endpoints.refresh_local_dapps()).

Even though the dapps are refreshed we should still re-try resolving the request.
Most likely a recursive call should be done here with some additional parameter preventing a refresh again.

tomusdrw · 2017-07-05T13:06:13Z

js/src/api/rpc/parity/parity.js

@@ -95,6 +95,11 @@ export default class Parity {
      .execute('parity_dappsList');
  }

+  dappsRefresh () {


I don't think the RPC method (and JS code) will be useful at all if the router can auto-refresh missing dapps.

Ah! Thanks for the review! I'll reply when I wake up in a few hours. The JS code just ensures an instant refresh.. otherwise you have to reload the page (+R), and I don't think we should require that. It would be confusing to the end user.

So for instance, if you link a new dapp... how would JS know unless it constantly checked if there was a new one... [When pub-sub is ready, I can change it] but this is incredibly inefficient since the number of times a developer is going to add a new dapp is like... once a month at most?

Yeah, I think refreshing the page is actually ok when you add a new dapp. As you said it will happen once a month at most.
IMHO not worth supporting additional RPC method and this additional JS code.

But I don't have very strong opinion on that. Since the code is already there we can probably merge it.

CraigglesO · 2017-07-15T03:00:08Z

The crazy thing is that the bug would only rear it's ugly head some cases. simplify filter push proves that. Anyways, the test cases would freeze on a let mut pages = self.endpoints.write() SOOOO Unless someone knows how to make that stop I'm making it verbose:

pub fn refresh_local_dapps(&self) {
		let mut local = self.local_endpoints.write();
		let new_local = apps::fs::local_endpoints(self.dapps_path.clone(), self.embeddable.clone());
		let (_, to_remove): (Vec<String>, Vec<String>) = local.clone()
			.into_iter()
			.partition(|k| new_local.contains_key(&k.clone()));
		// remove the dead dapps
		for k in to_remove {
			let index = local.iter().position(|x| *x == k).unwrap();
			local.remove(index);
			self.endpoints.write().remove(&k);
		}
		// new dapps to be added
		for (k, v) in new_local {
			if self.endpoints.write().contains_key(&k) != true {
				local.push(k.clone());
				self.endpoints.write().insert(k, v);
			}
		}
	}

tomusdrw · 2017-07-31T09:57:38Z

Waiting for #6192

tomusdrw · 2017-07-31T07:08:26Z

dapps/src/lib.rs

+			.partition(|k| new_local.contains_key(&k.clone()));
+		// remove the dead dapps
+		for k in to_remove {
+			let index = local.iter().position(|x| *x == k).unwrap();


unwrap -> expect

This makes the function quadratic and we don't really need it here. At the end of the function just replace self.local_endpoints with new_local.keys().collect().

tomusdrw · 2017-07-31T07:08:43Z

dapps/src/lib.rs

+		}
+		// new dapps to be added
+		for (k, v) in new_local {
+			if self.endpoints.read().contains_key(&k) != true {


Use !<condition> instead of comparing to bool.

tomusdrw · 2017-07-31T07:09:13Z

dapps/src/lib.rs

+		// new dapps to be added
+		for (k, v) in new_local {
+			if self.endpoints.read().contains_key(&k) != true {
+				local.push(k.clone());


That will be not needed when you do:
*local = new_local.keys().collect()

tomusdrw · 2017-07-31T07:20:33Z

dapps/src/router.rs

@@ -110,6 +112,7 @@ impl http::RequestMiddleware for Router {
 				=>
 			{
 				trace!(target: "dapps", "Resolving to 404.");
+				self.endpoints.as_ref().map(|endpoints| endpoints.clone().refresh_local_dapps());


It should retry resolving the dapp.

Addressing dapps-refresh grumbles from #5752

tomusdrw · 2017-08-01T10:14:55Z

I believe js/package-lock.json is produced by new NPM, right? I suppose we should finally check that in: @jacogr ?

CraigglesO added 3 commits June 1, 2017 08:16

RwLock

2d19a67

getting there

11f4fa5

argh

1c80221

CraigglesO added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 2, 2017

parking_lot

9adfdc0

rphmeier reviewed Jun 5, 2017

View reviewed changes

rphmeier added the M4-core ⛓ Core client code / Rust. label Jun 5, 2017

CraigglesO added 15 commits June 17, 2017 14:41

Merge branch 'master' of github.com:paritytech/parity into dapp-refresh

8428b23

rpc

c0e0022

Merge branch 'master' of github.com:paritytech/parity into dapp-refresh

baa2846

wax on wax off

ca573b8

almost there

83430c0

remove lock

25e5168

write over read

98a3eee

Merge branch 'master' of github.com:paritytech/parity into dapp-refresh

3815c17

works

5004dd3

linting

97ae381

small updates

09e7533

dissapearing act

95a90ff

router update

e7961b5

complete

f0d162d

one m

960731c

CraigglesO added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 5, 2017

CraigglesO requested a review from tomusdrw July 5, 2017 06:44

tomusdrw reviewed Jul 5, 2017

View reviewed changes

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 5, 2017

CraigglesO added 2 commits July 7, 2017 09:31

grumbles1

5a93a74

grumbles part II

d3fbfb8

CraigglesO added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 12, 2017

updates and the likes

0327eec

gavofyork added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 13, 2017

CraigglesO added 5 commits July 14, 2017 16:01

play

4b45b02

simplify filter

9242dc5

f-ing bugs

fea37ce

read->write

785bed3

Merge branch 'master' of github.com:paritytech/parity into dapp-refresh

faa4cc1

CraigglesO and others added 2 commits July 17, 2017 21:14

Merge branch 'master' of github.com:paritytech/parity into dapp-refresh

8c0123c

Address own grumbles.

dddd303

tomusdrw added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 31, 2017

tomusdrw suggested changes Jul 31, 2017

View reviewed changes

tomusdrw and others added 2 commits July 31, 2017 17:00

Fix test.

64633ec

Merge pull request #6192 from paritytech/dapps-refresh-fixes

004c9d3

Addressing dapps-refresh grumbles from #5752

tomusdrw approved these changes Aug 1, 2017

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Aug 1, 2017

tomusdrw added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 1, 2017

Merge branch 'master' into dapp-refresh

03a5eb1

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Aug 2, 2017

gavofyork merged commit 7d17d77 into master Aug 9, 2017

gavofyork deleted the dapp-refresh branch August 9, 2017 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dapp refresh #5752

Dapp refresh #5752

CraigglesO commented Jun 2, 2017 •

edited

Loading

rphmeier Jun 5, 2017

CraigglesO Jun 5, 2017

tomusdrw left a comment

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

CraigglesO Jul 5, 2017

tomusdrw Jul 5, 2017

rphmeier Jul 5, 2017

CraigglesO Jul 6, 2017 •

edited

Loading

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

CraigglesO Jul 6, 2017

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

tomusdrw Jul 5, 2017

CraigglesO Jul 5, 2017

tomusdrw Jul 5, 2017

CraigglesO commented Jul 15, 2017 •

edited

Loading

tomusdrw commented Jul 31, 2017

tomusdrw Jul 31, 2017

tomusdrw Jul 31, 2017

tomusdrw Jul 31, 2017

tomusdrw Jul 31, 2017

tomusdrw commented Aug 1, 2017

Dapp refresh #5752

Dapp refresh #5752

Conversation

CraigglesO commented Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CraigglesO Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CraigglesO commented Jul 15, 2017 • edited Loading

tomusdrw commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw commented Aug 1, 2017

CraigglesO commented Jun 2, 2017 •

edited

Loading

CraigglesO Jul 6, 2017 •

edited

Loading

CraigglesO commented Jul 15, 2017 •

edited

Loading