Announce by atomgardner · Pull Request #466 · casey/intermodal

atomgardner · 2020-09-06T12:50:48Z

Here's some alpha quality work towards #255

Please take note of the "popular repository" indelibly front and centre on my Microsoft Github profile page.

casey · 2020-09-07T03:51:12Z

Currently every top-level item is in its own file, and I'd like to keep doing that. It use fzf-vim to navigate around the project, and it makes it very easy to jump between structs by filename, and not have to search within a file.

What do you think about creating a tracker submodule in tracker.rs, and then renaming UdpTrackerConn to Client and putting it into src/tracker/client.rs? There might be an HTTP tracker in the future, or there might not be, so it seems unnecessary to plan in advance for it.

It makes sense to put request/response pairs in the the same file, so, for example, AnnounceRequest and AnnounceResponse can both go in src/tracker/announce.rs.

State can go in src/tracker/state.rs.

More comments in a second, but I wanted to start with that high level thought.

casey

I didn't get to reviewing the meat of the code, but it's getting late so I'm going to leave it at this for now.

If you check continuous integration, there are a bunch of clippy lints, you can see them locally if your run cargo clippy --all or just clippy.

casey · 2020-09-07T06:08:59Z

I also updated this branch with new commits from master, which I shouldn't have done, since it creates nasty merge commits. Feel free to rebase and force push.

atomgardner · 2020-09-08T12:15:43Z

The only problem I see with your high level thought: all of the big Linux distros use https trackers.

casey · 2020-09-08T21:45:41Z

The only problem I see with your high level thought: all of the big Linux distros use https trackers.

Sorry, I don't follow. If we add an HTTPS tracker later, can't the code be reorganized then?

atomgardner · 2020-09-09T14:49:30Z

Sorry, I don't follow. If we add an HTTPS tracker later, can't the code be reorganized then?

It totally can. I was stating the obvious fact that if imdl is to become super serious software, loved by users everywhere, an http tracker client must be included.

casey · 2020-09-09T18:13:33Z

It totally can. I was stating the obvious fact that if imdl is to become super serious software, loved by users everywhere, an http tracker client must be included.

100%!

atomgardner · 2020-09-14T21:39:07Z

This build pipeline is fastidious. Let's see if I can fix the book building.

casey · 2020-09-14T21:44:33Z

There's a lot of generated documentation, and a lot of checks that make sure that all the documentation is consistent and/or up-to-date.

I think in this case it's complaining that imdl torrent announce doesn't have an entry in bin/gen/config.yaml:

https://github.com/casey/intermodal/blob/master/bin/gen/config.yaml#L5

atomgardner · 2020-10-02T10:41:14Z

Interesting failure on the Windows machine. You totally saw it coming

casey · 2020-10-02T22:57:46Z

That's interesting! I'm surpised it would work on macos/linux, but fail on Windows.

atomgardner · 2020-10-11T07:47:24Z

I think Window is right to send back the error. Binding to 0.0.0.0:0 was definitely the wrong address.

casey · 2020-10-11T21:41:24Z

Ahh, yeah, I guess the tests should bind to 127.0.0.1?

casey

Made a bunch of comments, let me know what you think. Have you tried this with different tracker implementations, both OpenTracker, and anything else out there that's popular.

casey · 2020-10-20T04:15:15Z

  SymlinkRoot { root: PathBuf },
  #[snafu(display("Failed to retrieve system time: {}", source))]
  SystemTime { source: SystemTimeError },
+  #[snafu(display("The UDP tracker connection id has expired."))]


Hypothetically, shouldn't this not be a user-facing error, since the client is supposed to get a new connection ID if the old one expires?

For simplicity, I propose we deviate from the spec a little bit for now, instead of worrying about handling getting a new connection ID. For every request, let's do 4 attempts every 15 seconds, and if we don't get a response by the last attempt, we give up and return a TrackerTimeout error. This keeps us under the 60 second limit for our connection ID, and later we can implement exponential backoff and re-requesting new connection IDs.

casey · 2020-10-20T04:55:49Z

+    self.parse_compact_peer_list(&buf[mem::size_of::<announce::Response>()..len])
+  }
+
+  fn roundtrip<T: Request>(&self, req: &T, rxbuf: &mut [u8]) -> Result<(T::Response, usize)> {


I think I like fn exchange a little better than roundtrip, round trip sounds to me like the same thing going in both directions.

Instead of returning a usize, can send_and_retry_with_backoff and roundtrip return a &[u8], which is the written-to portion of the rxbuf? This would avoid needing to index into the rxbuf correctly.

I'll do a little research on this. But it looks like I left myself a little comment about this:

src/client/response.rs: pub(crate) trait Response { // We leak the response length so that payloads can be parsed. This should be // cleaner when associated types within traits can specify named lifetime // parameters. fn deserialize(buf: &[u8]) -> Result<(Self, &[u8])>

Woah—looks like this is easily doable now!!

atomgardner · 2020-12-27T02:59:14Z

Yo!! I've had a bunch of changes queued up for a while now, and I think this is getting closer to usable. Let me know how it's looking.

I'm also planning to push the Peer Protocol stuff up in the next few days.

casey · 2020-12-28T00:26:40Z

Awesome! It might take me a little bit, but I'll definitely get to this within the next few days.

casey

10 days is a few, right 😅 Sorry for taking so long! I did some initial review, just really minor stuff, because I realized that I don't remember where we were in the feature.

What's new that I should be focusing on?

casey · 2021-01-07T00:48:22Z

  code:    "imdl torrent --help"

+- command: imdl torrent announce
+  text:    "Announce a torrent to its trackers and print all peers returned:"


Minor reword: "Announce a torrent to trackers and print returned peers:"

casey · 2021-01-07T01:39:15Z

+  #[snafu(display("UDP tracker connection id has expired."))]
+  TrackerConnectionExpired,
+  #[snafu(display("UDP tracker resolved `{}` to no useable addresses.", host_port))]
+  TrackerNoHosts { host_port: HostPort },


Do you happen to know under what circumstances this could happen? It isn't obvious to me why to_socket_addrs would return an empty iterator, even though the stdlib docs say that it could.

My understanding of the DNS protocol is too shallow to know whether this behaviour is possible. It seems reasonable that whoever hatched to_socket_addrs probably spent some serious time thinking it all through.

casey · 2021-01-07T01:51:14Z

  clippy::map_unwrap_or,
  clippy::missing_docs_in_private_items,
  clippy::missing_inline_in_public_items,
+  clippy::needless_lifetimes,


I added this in a diff I just merged, so there'll be a conflict here.

atomgardner · 2021-01-07T08:30:07Z

Heya heya---when 10 days is a few days plus holidays, you don't count the holidays.

I've pushed up your copy changes, but, I think, the "integration test" needs a look. And you should also try the command and confirm that it works.

casey

Made a bunch of comments! Almost all of them requesting things that aren't tests. I created #490, to track things that don't belong in the initial PR.

I'm changing the tests to remove the one that generates the changelog, which means you also don't have to worry about adding a commit type to commits anymore. It was good for keeping the changelog up to date, but ultimately, I think, more annoying that it was worth.

If possible, could you push diffs instead of force pushing? This makes it easier to see what's changed since I last took a look. I think I might have said something about force pushing being preferable, but I take it all back.

casey · 2021-01-09T05:26:00Z

+        None => continue,
+      };
+
+      let client = tracker::Client::connect(&hostport);


This can be:

let client = match tracker::Client::connect(&hostport) { Err(e) => { errln!(env, "Couldn't connect to tracker: {}", e)?; continue; } Ok(client) => client, };

(I wish Rust had let = … else { … } statements like swift.)

casey · 2021-01-09T05:27:12Z

+        continue;
+      }
+
+      let hostport = match HostPort::from_url(&tracker) {


If a URL in the torrent is malformed, we should print a warning.

Also, there should be a test that such URLs are skipped.

casey · 2021-01-09T05:27:37Z

+    for result in metainfo.trackers() {
+      match result {
+        Ok(tracker) => trackers.push(tracker),
+        Err(err) => errln!(env, "Skipping malformed tracker URL: {}", err)?,


There should be a test that malformed tracker URLs are skipped.

casey · 2021-01-09T05:28:00Z

+    }
+
+    if trackers.is_empty() {
+      return Err(Error::MetainfoMissingTrackers);


There should be a test that elicits this error.

casey · 2021-01-09T05:28:19Z

+    }
+
+    for tracker in trackers {
+      if tracker.scheme() != "udp" {


There should be a test that non-UDP tracker URLs are skipped.

casey · 2021-01-09T05:45:20Z

+impl super::Request for Request {
+  type Response = Response;
+
+  fn serialize(&self) -> Vec<u8> {


This could use a test. Just had a thought: Request/response pairs should all have tests that round trip a struct, where the struct is serialized, deserialized, and the output asserted to be equal to the input.

casey · 2021-01-09T05:45:36Z

+
+impl super::Response for Request {
+  #[allow(dead_code)]
+  fn deserialize(buf: &[u8]) -> Result<(Self, usize)> {


This could use a test, and also one that elicits the error condition.

casey · 2021-01-09T05:45:55Z

+}
+
+impl super::Response for Response {
+  fn deserialize(buf: &[u8]) -> Result<(Self, usize)> {


This could use a test, including one for the error condition.

casey · 2021-01-09T05:46:04Z

+  type Response = Request;
+
+  #[allow(dead_code)]
+  fn serialize(&self) -> Vec<u8> {


This should be tested.

casey · 2021-01-09T05:52:08Z

@@ -0,0 +1,298 @@
+use super::*;


There should be tests that elicit all of the error conditions that are possible.

This command makes use of a partially-implemented UDP tracker client to announce an infohash and list the response. type: added

Switch `Into<X> for Y` to `From<Y> for X`.

Tidy up out-of-vogue `to_string()` usage.

Please MacOS by binding to `0.0.0.0`.

Tidy up exchange signature

Keep futzing with IP addresses in timeout tests.

Run `cargo +nightly fmt --all`

announce: print a warning when tracker URLs are malformed

announce: attempt to get a peer list from all trackers in the metainfo

Remove deadcode attribute

Follow clippy advice

Fix up bad diff from bad rebase.

Ditto

casey · 2022-11-19T05:30:14Z

I think this has gone stale 😅

casey requested changes Sep 7, 2020

View reviewed changes

casey reviewed Sep 8, 2020

View reviewed changes

Comment thread src/udp_tracker.rs Outdated

atomgardner force-pushed the announce branch 4 times, most recently from 360ac58 to d0f5b7d Compare September 14, 2020 11:08

atomgardner force-pushed the announce branch 2 times, most recently from 8e3e73b to b9ef0a3 Compare September 15, 2020 00:23

atomgardner force-pushed the announce branch from 112385f to 6239286 Compare October 2, 2020 10:41

atomgardner force-pushed the announce branch 2 times, most recently from 885f949 to 6673c2b Compare October 11, 2020 07:09

atomgardner force-pushed the announce branch 2 times, most recently from 670c69a to 4b00703 Compare October 17, 2020 01:37

casey reviewed Oct 20, 2020

View reviewed changes

Comment thread bin/gen/config.yaml

casey requested changes Oct 20, 2020

View reviewed changes

atomgardner force-pushed the announce branch from 4b00703 to e6b05c1 Compare December 27, 2020 02:38

atomgardner requested a review from casey December 27, 2020 02:38

atomgardner force-pushed the announce branch from e6b05c1 to 292f9b0 Compare December 27, 2020 02:47

casey mentioned this pull request Jan 7, 2021

Include domain, IP address, and port in network error messages #489

Open

casey requested changes Jan 7, 2021

View reviewed changes

atomgardner force-pushed the announce branch 2 times, most recently from 91a5ff8 to c4237c0 Compare January 7, 2021 08:28

casey requested changes Jan 9, 2021

View reviewed changes

atomgardner added 5 commits August 8, 2021 20:27

Add a torrent announce subcommand

19c6fb3

This command makes use of a partially-implemented UDP tracker client to announce an infohash and list the response. type: added

squash! Add a torrent announce subcommand

158336e

squash! Add a torrent announce subcommand

007c42f

squash! Add a torrent announce subcommand

ba782e8

Switch `Into<X> for Y` to `From<Y> for X`.

squash! Add a torrent announce subcommand

267912b

Tidy up out-of-vogue `to_string()` usage.

atomgardner force-pushed the announce branch from 7d3c8e8 to 267912b Compare August 8, 2021 11:04

atomgardner added 10 commits August 8, 2021 21:30

squash! Add a torrent announce subcommand

027010b

Please MacOS by binding to `0.0.0.0`.

squash! Add a torrent announce subcommand

12d5a95

Tidy up exchange signature

squash! Add a torrent announce subcommand

63797eb

Keep futzing with IP addresses in timeout tests.

squash! Add a torrent announce subcommand

03b416f

Run `cargo +nightly fmt --all`

squash! Add a torrent announce subcommand

afa994f

announce: print a warning when tracker URLs are malformed

squash! Add a torrent announce subcommand

b7ff9d8

announce: attempt to get a peer list from all trackers in the metainfo

squash! Add a torrent announce subcommand

1b781e6

Remove deadcode attribute

squash! Add a torrent announce subcommand

e6f7dbe

Follow clippy advice

squash! Add a torrent announce subcommand

44597eb

Fix up bad diff from bad rebase.

squash! Add a torrent announce subcommand

3089755

Ditto

casey closed this Nov 19, 2022

Conversation

atomgardner commented Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casey commented Sep 7, 2020

Uh oh!

casey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

casey commented Sep 7, 2020

Uh oh!

atomgardner commented Sep 8, 2020

Uh oh!

Uh oh!

casey commented Sep 8, 2020

Uh oh!

atomgardner commented Sep 9, 2020

Uh oh!

casey commented Sep 9, 2020

Uh oh!

atomgardner commented Sep 14, 2020

Uh oh!

casey commented Sep 14, 2020

Uh oh!

atomgardner commented Oct 2, 2020

Uh oh!

casey commented Oct 2, 2020

Uh oh!

atomgardner commented Oct 11, 2020

Uh oh!

casey commented Oct 11, 2020

Uh oh!

Uh oh!

casey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atomgardner commented Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

casey commented Dec 28, 2020

Uh oh!

casey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

atomgardner commented Sep 6, 2020 •

edited

Loading

atomgardner commented Dec 27, 2020 •

edited

Loading