Reuse socket #72

raphjaph · 2022-10-26T18:53:52Z

This is a quick and dirty attempt at fixing #67 that reuses the socket for multiple requests. It has few issues:

no error handing, client becomes unusable after encountering a single error
SimpleHttpTransport::request() takes a mut reference to self, which cascades to all downstream users
TcpStream is not Clone, therefore I had to remove the derive Clone

An alternate implementation based on reqwest (hyper seems too low-level because of the async stuff) wouldn't have these problems. So I think I'm going to start on that. Just wanted so share this draft PR if anyone has feedback or has an idea on how to do this without reqwest.

apoelstra · 2022-10-26T21:40:54Z

Cool! Thanks for taking this on.

Honestly I think you're close to having a "good" design. A couple small changes would fix your limitations:

To get error handling, catch errors and set self.sock to None before returning; now the client can be reused. A later iteration should maybe detect EINTR and retry sending automatically, or something, but we can deal with that later.
To avoid &mut and get Clone we could wrap the underlying socket in a Arc<Mutex>...but OTOH the user can do that themselves, which is a minor annoyance and gives the user more freedom.

raphjaph · 2022-10-27T17:49:10Z

Thanks for the tips! Have a look and let me know what you think.

apoelstra · 2022-10-27T18:15:24Z

Could you squash all the commits into one?

src/simple_http.rs

apoelstra

ACK 9f4ab54

raphjaph · 2022-10-28T20:16:08Z

We tried using this and something went wrong, so definitely don't merge this yet. I'll try to take a look at this over the weekend and figure out what exactly went wrong.

raphjaph · 2022-10-31T21:30:15Z

There were two problems:

The Connection: Close HTTP Header was set. This would tell the RPC server to close the connection after it finished writing, so subsequent socket writes would beget broken pipe errors.
We needed to parse the Content-Length Header to know when the response was done.

We tested it with our application and everything works as it should now. So this would also close #67.

tcharding

ACK c621b97

src/simple_http.rs

tcharding · 2022-11-01T22:26:42Z

ACK 1826d5e

tcharding · 2022-11-16T03:04:41Z

Gentle bump @apoelstra

apoelstra · 2022-11-16T13:46:04Z

Yep, sorry -- let me test resyncing my homebrew wallet against Core with this change and then it can go in.

apoelstra · 2022-11-17T14:24:03Z

src/simple_http.rs


+        let content_length = content_length.ok_or(Error::HttpParseError)?;
+
+        let mut buffer = vec![0; content_length];


We definitely need to put a limit on this, but it's fine for now :). We'll do a followup PR to audit this because I think get_line may also be subject to memory exhaustion attacks.

yes, that sounds prudent

apoelstra

ACK 1826d5e

apoelstra · 2022-11-17T14:35:59Z

Hmm, with this PR I see a dramatic (~10x) slowdown in my syncing speed. top shows around 20% CPU usage from each of icboc (my wallet) and bitcoind, i.e. both are only using one core. Vs with the commit before this PR both are close to 50%.

My ACK still stands because we need to fix the Mac bug but we need to investigate this.

apoelstra · 2022-11-17T14:46:46Z

Edit: a previous version of this comment said that there was still a 50-100% perf hit. This is because my laptop was unplugged and therefore throttled when I was doing the measurement.

Ok, after modifying this PR to tighten the mutex lock's scope, in particular to unlock it before doing any parsing of the result, I get the same performance as before (possibly a small speedup, but I'm not measuring carefully enough to tell). I'm fine to move that to a followup PR as well.

tcharding · 2022-11-17T20:37:03Z

@raphjaph just fyi your CI fails are not your fault, will be resolved by: #74 (comment)

apoelstra · 2022-11-17T20:58:38Z

@raphjaph can you rebase on master? We've fixed the CI.

raphjaph · 2022-11-17T22:09:23Z

Edit: a previous version of this comment said that there was still a 50-100% perf hit. This is because my laptop was unplugged and therefore throttled when I was doing the measurement.

Ok, after modifying this PR to tighten the mutex lock's scope, in particular to unlock it before doing any parsing of the result, I get the same performance as before (possibly a small speedup, but I'm not measuring carefully enough to tell). I'm fine to move that to a followup PR as well.

Ok, that's good to know I'll do the performance stuff in a follow-up PR then.

apoelstra

ACK e867374

2380d90 simple_http: eliminate confused `get_line` function; reuse buffer (Andrew Poelstra) 72bea34 simple_http: deserialize json from the reader, don't use an intermediate buffer (Andrew Poelstra) c20033e simple_http: add type ascriptions; fix incorrect mutexguard drop logic (Andrew Poelstra) 20da231 simple_http: store the BufReader rather than bare socket (Andrew Poelstra) 8d2ec83 add fuzzing harness for simple_http (Andrew Poelstra) f44a535 simple_http: mock out TcpStream when fuzzing (Andrew Poelstra) 2a0903e simple_http: handle large or missing Content-length headers correctly (Andrew Poelstra) e6eaedd simple_http: fix crash when receiving non-ascii HTTP hello (Andrew Poelstra) 7640a4b simple_http: split `HttpParseError` into several variants (Andrew Poelstra) 36708d6 simple_http: tighten mutex lifetime (Andrew Poelstra) Pull request description: Tightens the mutex lifetime to improve performance. Basically we read all the data into a buffer and then unlock the mutex ASAP. We could be a bit more memory-efficient by using `serde_json::from_reader` to parse directly from the socket, but (a) that would make it harder to enforce the Content-length header; and (b) it'd hold the mutex for longer than necessary. This commit also splits out the `HttpParseError` into several variants, which unfortunately makes this a breaking change. I think I can move that commit into its own PR if we want to get a minor rev out, but I don't think we care too much about that since this crate is pretty far to the edge of most dependency trees. Adds a fuzz harness but doesn't integrate it into CI. Fixes a couple bugs I found while fuzzing locally. ACKs for top commit: raphjaph: ACK 2380d90 tcharding: ACK 2380d90 Tree-SHA512: a7462434606f193c0736ebb74cb516d121dbd315db47a82754abb236b47ed744c2655729c407a98613e468f48a2fac025e5ccdab69e1e89395aeb0c2b2dfefca

raphjaph marked this pull request as ready for review October 27, 2022 17:49

raphjaph mentioned this pull request Oct 27, 2022

Socket or port exhaustion when making fast RPC calls to bitcoin core ordinals/ord#309

Closed

apoelstra reviewed Oct 27, 2022

View reviewed changes

src/simple_http.rs Outdated Show resolved Hide resolved

apoelstra reviewed Oct 27, 2022

View reviewed changes

src/simple_http.rs Outdated Show resolved Hide resolved

apoelstra reviewed Oct 27, 2022

View reviewed changes

src/simple_http.rs Outdated Show resolved Hide resolved

raphjaph force-pushed the reuse-socket branch from 488c81a to 9f4ab54 Compare October 27, 2022 19:35

apoelstra approved these changes Oct 28, 2022

View reviewed changes

raphjaph force-pushed the reuse-socket branch 2 times, most recently from 697fcb4 to c621b97 Compare October 31, 2022 21:29

tcharding approved these changes Oct 31, 2022

View reviewed changes

src/simple_http.rs Outdated Show resolved Hide resolved

raphjaph force-pushed the reuse-socket branch from c621b97 to 1826d5e Compare November 1, 2022 13:38

apoelstra reviewed Nov 17, 2022

View reviewed changes

apoelstra approved these changes Nov 17, 2022

View reviewed changes

Reuse HTTP connection

e867374

raphjaph force-pushed the reuse-socket branch from 1826d5e to e867374 Compare November 17, 2022 22:07

apoelstra approved these changes Nov 17, 2022

View reviewed changes

apoelstra merged commit fe53ba4 into apoelstra:master Nov 17, 2022

apoelstra mentioned this pull request Nov 19, 2022

Simple HTTP cleanups and followup to #72 #76

Merged

apoelstra mentioned this pull request Nov 28, 2022

Timeout on MacOS after making ~16384 RPC calls #67

Closed

w0xlt mentioned this pull request Jan 29, 2023

Update jsonrpc to 0.14 rust-bitcoin/rust-bitcoincore-rpc#276

Open

tnull mentioned this pull request Feb 15, 2023

Make test_esplora_syncs more robust lightningdevkit/rust-lightning#2033

Merged

rajarshimaitra mentioned this pull request Feb 16, 2023

[RPC] Ignore importdescriptor error bitcoindevkit/bdk#862

Closed

4 tasks


		let content_length = content_length.ok_or(Error::HttpParseError)?;

		let mut buffer = vec![0; content_length];

Reuse socket #72

Reuse socket #72

Uh oh!

Conversation

raphjaph commented Oct 26, 2022

Uh oh!

apoelstra commented Oct 26, 2022

Uh oh!

raphjaph commented Oct 27, 2022

Uh oh!

apoelstra commented Oct 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

raphjaph commented Oct 28, 2022

Uh oh!

raphjaph commented Oct 31, 2022

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding commented Nov 1, 2022

Uh oh!

tcharding commented Nov 16, 2022

Uh oh!

apoelstra commented Nov 16, 2022

Uh oh!

apoelstra Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

raphjaph Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Nov 17, 2022

Uh oh!

apoelstra commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Nov 17, 2022

Uh oh!

apoelstra commented Nov 17, 2022

Uh oh!

raphjaph commented Nov 17, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

apoelstra commented Nov 17, 2022 •

edited

Loading