Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unwraps in network code #37

Closed
sashahilton00 opened this issue Jan 29, 2018 · 6 comments
Closed

unwraps in network code #37

sashahilton00 opened this issue Jan 29, 2018 · 6 comments
Labels

Comments

@sashahilton00
Copy link
Member

Issue by crepererum
Saturday Nov 12, 2016 at 17:32 GMT
Originally opened as plietar/librespot#127


The network-related code (e.g. in session.rs) uses unwrap, even in places where failure is kinda likely. For example:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: IoError(Error { repr: Os { code: 104, message: "Connection reset by peer" } })', /buil
dslave/rust-buildbot/slave/stable-dist-rustc-cross-host-linux/build/src/libcore/result.rs:799
stack backtrace:
   1:       0x557f3db8d3 - std::sys::backtrace::tracing::imp::write::hd28a1f8221c946e5
   2:       0x557f3dfd9b - std::panicking::default_hook::{{closure}}::hcb904beb56706ec8
   3:       0x557f3de75b - std::panicking::default_hook::hd59cb475a55dc643
   4:       0x557f3dedcf - std::panicking::rust_panic_with_hook::hea2bde53d708eb9a
   5:       ne557f3dec9b - std::panicking::begin_panic::hc4949303f890336e
   6:       0x557f3debeb - std::panicking::begin_panic_fmt::he756d57ad1c1864c
   7:       0x557f3deb8b - rust_begin_unwind
   8:       0x557f40825b - core::panicking::panic_fmt::ha615cb8b8c64841b
   9:       0x557f1a9123 - core::result::unwrap_failed::hf088160fd5523cfd
  10:       0x557f1f211b - librespot::session::Session::recv::h1029c0727cfdd2f5
  11:       0x557f1f1bb7 - librespot::session::Session::poll::h8d6b905a3d50b9e1
  12:       0x557f16f5db - librespot::main::ha3ae70879a6a6aa7
  13:       0x557f3e512f - __rust_maybe_catch_panic
  14:       0x557f3ddedb - std::rt::lang_start::hcb77440d8735d978
  15:       0x7fa1c4a363 - __libc_start_main
  2::       0x557f167363 - <unknown>

Connection resets are a normal thing, esp. in Wifi-environments. So the recv method should probably return a Result-type instead and the layer above (poll) should handle failed connection attempts, e.g. by reconnect/retry.

@sashahilton00
Copy link
Member Author

Comment by crepererum
Saturday Nov 12, 2016 at 17:33 GMT


Apart from it, I can happily report that librespot runs perfectly on a ODROID-C2, which is an AArch64 platform 😃

@sashahilton00
Copy link
Member Author

Comment by plietar
Saturday Nov 12, 2016 at 17:53 GMT


Yeah, I wrote librespot while I was still reverse engineering the protocol, so these issues weren't really my priority.

There's a lot of things I'm not too happy with in the code, including this, but I don't really have much time to work on it. One day though ...

Glad to hear it still works 😄

@sashahilton00
Copy link
Member Author

Comment by plietar
Saturday Nov 19, 2016 at 21:49 GMT


FYI, I'm working on a (partial) rewrite which should be much better at handling network issues

@sashahilton00
Copy link
Member Author

Comment by joerg-krause
Tuesday Jan 10, 2017 at 19:22 GMT


I am getting the "Connection reset by peer", too. Any idea why the connection is reset? How can the panic be handled properly?

@sashahilton00
Copy link
Member Author

Comment by crepererum
Friday Jan 13, 2017 at 06:47 GMT


I guess the reset means that either the Spotify server directly or some node in between cuts the TCP connection. This ain't uncommon, expecially when the server suffers under high loads. Network problems are (IMHO) such normal that they shouldn't produce a panic. But as @plietar already mentioned, proof of concept code does hit always follow this rule.

@ComlOnline
Copy link
Contributor

Please see #103 for more on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants