fix: Deal with concurrent hub access better #245
Conversation
sentry-log/src/integration.rs
Outdated
| INIT.call_once(|| { | ||
| // silently ignore errors here, as running tests will hit this condition for some reason. | ||
| log::set_boxed_logger(Box::new(Logger::default())).ok(); | ||
| log::set_boxed_logger(Box::new(Logger::default())).unwrap(); |
There was a problem hiding this comment.
Just in case someone already set the logger, should we still .ok() here, or even log the error? Even if this is a static programming error, it's a bit harsh to panic here.
There was a problem hiding this comment.
We had the unwrap here originally, which I changed just recently: 97b9006#diff-8c955be4e92f6bf80501c6237c378f5e
After looking into this with @mitsuhiko we think it was rather a problem of libbacktrace segfaulting on us. Since we should rather fail hard in case of misconfiguration rather than silently not do anything, I think its okay to unwrap here, in the case the user has really configured the logger twice.
There was a problem hiding this comment.
Well, the original log::set_boxed_logger also doesn't panic and rather errors. The problem is that it's much harder to recover from a panic than from an error, and our SDK should never crash user apps, even when misconfigured. I'd rather log an error and disable the integration than make the user application crash. Imagine this is only triggered with production configuration for some customers and then hits them where it hurts them most.
I will do some more tests also on windows where I was also seeing this problem. |
This should fix #237, and we should backport this to 0.19 as well.