refactor (hardware wallet) : reduce the number of threads #9644

niklasad1 · 2018-09-25T08:56:58Z

Attempt to close #9622 along with fixing some minor formatting things!

It introduces some boiler-plate code though such as:

Some errors are duplicated in trezor and ledger but I think these makes sense because it indicate the errors more clearly
Device arrived and left methods
Checking the device is valid
Fixed the Ledger logs which was faulty (added inline comments for more info)

Tested successfully both on Trezor and Ledger on Linux

Proof that number of threads have been reduced:

# This branch
$  ps hH p 19919 | wc -l
57
# master
$ ps hH p 23329 | wc -l 
58

niklasad1 · 2018-09-25T08:59:11Z

hw/src/ledger.rs

-	pub fn new(hidapi: Arc<Mutex<hidapi::HidApi>>, exiting: Arc<AtomicBool>) -> Result<Arc<Self>, libusb::Error> {
-		let manager = Arc::new(Self {
-			usb: hidapi,
+	pub fn new(usb: Arc<Mutex<hidapi::HidApi>>) -> Arc<Self> {


doesn't make sense that this function returns Result anymore because it can't fail anymore

niklasad1 · 2018-09-25T08:59:54Z

hw/src/ledger.rs

-			.ok();
-
-		Ok(manager)
+		})
 	}

 	// Transport Protocol:


fix formatting!!!

5chdn · 2018-10-26T11:27:10Z

@niklasad1 what's the status of this? Do you want feedback from @dvdplm @debris ?

niklasad1 · 2018-10-26T11:30:21Z

@5chdn Need grab to hardware wallets and test this first. I will change the label when it is good for reviewing hopefully later today

sorpaas

LGTM!

hw/src/ledger.rs

hw/src/lib.rs

5chdn · 2018-12-05T09:37:56Z

👍 needs a 2nd review

dvdplm

A few nits, but overall looks good.

hw/src/lib.rs

dvdplm · 2018-12-14T13:55:35Z

hw/src/lib.rs

+/// Hardware wallet event handler
+///
+/// Note, that this run to completion and race-conditions can't occur but this can
+/// therefore starve other events for being process with a spinlock or similar


Suggested change

/// therefore starve other events for being process with a spinlock or similar

/// stop other events from being processed with a spinlock or similar.

(but I'm not sure what you mean by "with spinlock or similar" tbh)

Spinlock is a bit blurry in this context, should probably say spinning instead.

I mean that if the thread is in an infinite loop or looping for a condition it will prevent other events from being processed. I can rephrase it but not really introduced in this PR

dvdplm · 2018-12-14T14:00:53Z

hw/src/lib.rs

+impl libusb::Hotplug for EventHandler {
+	fn device_arrived(&mut self, device: libusb::Device) {
+		// Upgrade reference to an Arc
+		if let (Some(ledger), Some(trezor)) = (self.ledger.upgrade(), self.trezor.upgrade()) {


q: is it impossible to have one upgrade call return Some and the other None? Wouldn't that cause the whole block to be skipped? In other words, are both upgrade() calls going to return Some if one of them does?

Good question, because the HardwareWalletManager keeps an Arc for both Ledger and Trezor for the entire lifetime of Parity unless it is disabled by the cli arg --no-hardware-wallet! upgrade()will always succeed as long theHardwareWalletManageris enabled (the strong count will be at least 1). So, either bothTrezorandLedger` is enabled or none is enabled.

However, I admit it is a bit sloppy. Theoretically, interleaving "could" happen if one of the hw-wallets has been dropped and the other has not been dropped but this is matter in granularity in terms of microseconds and I don't worry too much about this.

I can change it if you like? :P

dvdplm · 2018-12-14T14:11:11Z

hw/src/lib.rs

+
+	fn device_left(&mut self, device: libusb::Device) {
+		// Upgrade reference to an Arc
+		if let (Some(ledger), Some(trezor)) = (self.ledger.upgrade(), self.trezor.upgrade()) {


Same question here.

hw/src/lib.rs

hw/src/ledger.rs

5chdn · 2019-01-02T10:17:38Z

Please resolve conflicts :)

This changes parity to use one thread instead of two to subscribe USB events via the hardware-wallets.

Co-Authored-By: niklasad1 <[email protected]>

* Clarify bad explaination of `run-to-completion` * Replace magic numbers with constants

niklasad1 added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Sep 25, 2018

niklasad1 commented Sep 25, 2018

View reviewed changes

hw/src/ledger.rs Outdated

.ok();

Ok(manager)

})

}

// Transport Protocol:

Copy link

Collaborator Author

niklasad1 Sep 25, 2018

Choose a reason for hiding this comment

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

fix formatting!!!

5chdn added this to the 2.2 milestone Sep 25, 2018

5chdn added the M4-core ⛓ Core client code / Rust. label Sep 25, 2018

5chdn requested review from debris and dvdplm October 26, 2018 11:26

5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 26, 2018

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from 013abd3 to aacb057 Compare October 27, 2018 09:25

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Oct 28, 2018

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from 334c6fb to c26a70f Compare October 28, 2018 20:34

5chdn modified the milestones: 2.2, 2.3 Oct 29, 2018

sorpaas approved these changes Nov 25, 2018

View reviewed changes

hw/src/ledger.rs Outdated Show resolved Hide resolved

hw/src/lib.rs Outdated Show resolved Hide resolved

sorpaas added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 25, 2018

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from c26a70f to 79ba32d Compare November 25, 2018 21:16

dvdplm approved these changes Dec 14, 2018

View reviewed changes

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch 3 times, most recently from 09649d6 to fa2324a Compare December 16, 2018 20:13

niklasad1 removed the request for review from debris December 16, 2018 20:14

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from fa2324a to f099f3d Compare December 17, 2018 09:39

niklasad1 added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Dec 17, 2018

niklasad1 and others added 7 commits January 2, 2019 14:18

refactor(hardware-wallet) - use only one thread

b455fcf

This changes parity to use one thread instead of two to subscribe USB events via the hardware-wallets.

Fix logs and tests

1f16984

fix(grumbles) : formatting and spelling nits

2b20a2b

Update hw/src/lib.rs

0b6b7ae

Co-Authored-By: niklasad1 <[email protected]>

Update hw/src/lib.rs

75f87f8

Co-Authored-By: niklasad1 <[email protected]>

Update hw/src/ledger.rs

8f152b9

Co-Authored-By: niklasad1 <[email protected]>

fix(grumbles): clarify docs and use constants

0329802

* Clarify bad explaination of `run-to-completion` * Replace magic numbers with constants

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from f099f3d to 0329802 Compare January 2, 2019 13:19

5chdn merged commit 2bb7961 into master Jan 2, 2019

5chdn deleted the refactor/hardware-wallet-one-thread branch January 2, 2019 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor (hardware wallet) : reduce the number of threads #9644

refactor (hardware wallet) : reduce the number of threads #9644

niklasad1 commented Sep 25, 2018 •

edited

Loading

niklasad1 Sep 25, 2018 •

edited

Loading

niklasad1 Sep 25, 2018

5chdn commented Oct 26, 2018

niklasad1 commented Oct 26, 2018

sorpaas left a comment

5chdn commented Dec 5, 2018

dvdplm left a comment

dvdplm Dec 14, 2018

niklasad1 Dec 15, 2018

dvdplm Dec 14, 2018

niklasad1 Dec 15, 2018

dvdplm Dec 14, 2018

5chdn commented Jan 2, 2019

	/// therefore starve other events for being process with a spinlock or similar
	/// stop other events from being processed with a spinlock or similar.

refactor (hardware wallet) : reduce the number of threads #9644

refactor (hardware wallet) : reduce the number of threads #9644

Conversation

niklasad1 commented Sep 25, 2018 • edited Loading

niklasad1 Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

niklasad1 Sep 25, 2018

Choose a reason for hiding this comment

5chdn commented Oct 26, 2018

niklasad1 commented Oct 26, 2018

sorpaas left a comment

Choose a reason for hiding this comment

5chdn commented Dec 5, 2018

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm Dec 14, 2018

Choose a reason for hiding this comment

niklasad1 Dec 15, 2018

Choose a reason for hiding this comment

dvdplm Dec 14, 2018

Choose a reason for hiding this comment

niklasad1 Dec 15, 2018

Choose a reason for hiding this comment

dvdplm Dec 14, 2018

Choose a reason for hiding this comment

5chdn commented Jan 2, 2019

niklasad1 commented Sep 25, 2018 •

edited

Loading

niklasad1 Sep 25, 2018 •

edited

Loading