split into two crates/use async #102
Conversation
|
Went with async + tokio because I wanted to use hyper which seems like the best low level http library. Some of the higher level clients I used had trouble with things like accepting http 0.9 |
| Poll::Ready(Some(Ok(frame))) => { | ||
| let chunk = frame.into_data(); | ||
| if let Some(chunk) = &chunk { | ||
| tracing::trace!(length = chunk.len(), "recv"); |
There was a problem hiding this comment.
Are these impl blocks here primarily to introduce logging?
There was a problem hiding this comment.
This one is because body::Incoming doesn't implement Stream (I think because of trailer response headers, but we don't have to deal with those in ntrip afaik). The one on SendHalf is just forwarding to the Sink impl on mpsc::Sender. That is just to hide the channel from the public API, could also return the mpsc::Sender<Bytes> directly, or an impl Sink instead of a named SendHalf. Opted against mpsc::Sender because it's an implementation detail, and I like a named type because we can add extra methods to it, vs needing an extension trait like with sbp::iter_messages
| }) | ||
| .compact() | ||
| .init(); | ||
| tokio::runtime::Builder::new_current_thread() |
There was a problem hiding this comment.
Default behavior has changed, with no user provided switches we now attempt to connect to Skylark and return error 403. We should return the old behavior of doing nothing, or emit a help message if they don't specify anything.
There was a problem hiding this comment.
The current default behavior is to try and connect as well
➜ ntripping git:(steve/updates) ✗ ntripping --verbose
* Trying 50.112.176.233:2101...
* Connected to na.skylark.swiftnav.com (50.112.176.233) port 2101 (#0)
> GET /CRS HTTP/1.1
Host: na.skylark.swiftnav.com:2101
User-Agent: NTRIP ntrip-client/1.0
Accept: */*
Ntrip-Version: Ntrip/2.0
X-SwiftNav-Client-Id: 00000000-0000-0000-0000-000000000000
Expect: 100-continue
* Mark bundle as not supporting multiuse
< HTTP/1.1 401 Unauthorized
< Connection: close
<
{ * Closing connection 0
➜ ntripping git:(steve/updates) ✗ echo $?
0
But right now this version returns a non-zero status code
Finished dev [unoptimized + debuginfo] target(s) in 0.05s
Running `target/debug/ntripping -v`
2022-12-15T22:56:06.736849Z DEBUG ntripping: connect addr=na.skylark.swiftnav.com:2101
2022-12-15T22:56:07.103324Z DEBUG hyper::proto::h1::io: flushed 215 bytes
2022-12-15T22:56:07.139357Z DEBUG hyper::proto::h1::io: parsed 1 headers
2022-12-15T22:56:07.139480Z DEBUG hyper::proto::h1::conn: incoming body is close-delimited
Error: BadStatus(401)
➜ ntripping git:(steve/updates) ✗ echo $?
1
But yeah the default behavior probably should exit successfully
There was a problem hiding this comment.
Is there a public endpoint we could default to? Maybe something that returns a source table?
There was a problem hiding this comment.
The current default behavior is to try and connect as well
Hrmm, guess I never noticed it was trying to connect because it was returning success
There was a problem hiding this comment.
Is there a public endpoint we could default to? Maybe something that returns a source table?
This returns a source table
There was a problem hiding this comment.
Ah right, forgot that isn't protected by the auth
| fn main() -> Result<()> { | ||
| let opt = Cli::parse(); | ||
| tracing_subscriber::fmt::fmt() | ||
| .with_max_level(match opt.verbose { |
There was a problem hiding this comment.
For this to work, do we need to set RUST_LOG and add a logger (or is the tracing subscriber supposed to handle this)? env_logger? As of right now the -v option isn't showing anything for me.
There was a problem hiding this comment.
It shouldn't need it set, try -vv? I guess I have nothing logging at the debug level
There was a problem hiding this comment.
Actually looks like I do, and I see them with just -v
Finished dev [unoptimized + debuginfo] target(s) in 0.05s
Running `target/debug/ntripping -v`
2022-12-15T22:54:51.013819Z DEBUG ntripping: connect addr=na.skylark.swiftnav.com:2101
There was a problem hiding this comment.
A little concerned that we're losing some user facing features, like connection logging from cURL:
Versus:
So we're losing logging around a few things:
- Response headers
- GGA/CRA string sent up
- Connection state
There was a problem hiding this comment.
There is not rush on this probably should have made this pr a draft. the metrics project I wanna use this for just got pushed back again. Just have been missing rust a bit
I also still want to add
- connection pooling so requests to different mountpoints but the same host share a connection
- automatic retry, plus a way to hook into the retries (so you can, for example, have the retry send a CRA which a certain correction mask)
- maybe sourcetable types? I was thinking it might be nice to just point this too-be-built app at a caster and have it discover what mountpoints to monitor
So definitely can make it feature complete with the old version. Could also try a version with curl, but crate we use now is pretty much all !Send + !Sync which is a little annoying
There was a problem hiding this comment.
Maybe https://docs.rs/isahc/latest/isahc/ would work as an alternative? It's purports to be a more "Rusty" wrapper of cURL.
There was a problem hiding this comment.
Looks like there's no easy way to enable HTTP 0.9 in this library though, so might have to be hacked in.
There was a problem hiding this comment.
Yeah that's the first library I tried but saw it had and old open issue about that, but maybe it wouldn't be too hard to add
Hoping to do the monitoring project in rust, and for that it'd be nice to have a separate library