Skip to content

Commit

Permalink
fix: don't eat WebpushSocket poll_complete errors (#374)
Browse files Browse the repository at this point in the history
instead check specifically for the harmless connection closed error and don't
log it

also remove the stacktrace from ApcError::to_string, include it in the sentry
event exception instead

Issue SYNC-3444
  • Loading branch information
pjenvey authored May 2, 2023
1 parent 1952559 commit f8e6525
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 22 deletions.
12 changes: 1 addition & 11 deletions autopush-common/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,9 @@ pub struct ApcError {
pub backtrace: Box<Backtrace>,
}

// Print out the error and backtrace, including source errors
impl Display for ApcError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "Error: {}\nBacktrace: \n{:?}", self.kind, self.backtrace)?;

// Go down the chain of errors
let mut error: &dyn std::error::Error = &self.kind;
while let Some(source) = error.source() {
write!(f, "\n\nCaused by: {source}")?;
error = source;
}

Ok(())
self.kind.fmt(f)
}
}

Expand Down
3 changes: 3 additions & 0 deletions autopush/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,9 @@ where
let error = if let Some(ref err) = error {
let ua_info = ua_info.clone();
let mut event = sentry::event_from_error(err);
event.exception.last_mut().unwrap().stacktrace =
sentry::integrations::backtrace::backtrace_to_stacktrace(&err.backtrace);

event.user = Some(sentry::User {
id: Some(webpush.uaid.as_simple().to_string()),
..Default::default()
Expand Down
19 changes: 8 additions & 11 deletions autopush/src/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,12 +406,14 @@ impl Server {
handle.spawn(client.then(move |res| {
srv.open_connections.set(srv.open_connections.get() - 1);
if let Err(e) = res {
let mut errstr: &str = &e.to_string();
if errstr.contains("Connection closed normally") {
// we don't need the stack for a Success Error.
errstr = errstr.split('\n').collect::<Vec<&str>>()[0];
// No need to log ConnectionClosed (denotes the
// connection closed normally)
if !matches!(
e.kind,
ApcErrorKind::Ws(tungstenite::error::Error::ConnectionClosed)
) {
debug!("🤫 {}: {}", addr, e.to_string());
}
debug!("🤫 {}: {}", addr, errstr);
}
Ok(())
}));
Expand Down Expand Up @@ -909,14 +911,9 @@ where
}

/// Handle the poll completion.
/// Note, that this eats the errors of poll_complete(), because one of the known states
/// is "Error: Connection closed normally" which can raise a false error condition.
fn poll_complete(&mut self) -> Poll<(), ApcError> {
try_ready!(self.send_ws_ping());
if self.inner.poll_complete().is_err() {
warn!("Error encountered with poll_complete");
}
Ok(Async::Ready(()))
Ok(self.inner.poll_complete()?)
}

fn close(&mut self) -> Poll<(), ApcError> {
Expand Down

0 comments on commit f8e6525

Please sign in to comment.