fix: silence dbus errors #20939
fix: silence dbus errors #20939
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| // credential wallet (none exists in a docker container) and won't show up in the system tray (again, none exists). | ||
| // Failure to connect is expected and normal here, but users frequently misidentify these errors as the cause of their problems. | ||
|
|
||
| // https://github.com/cypress-io/cypress/issues/19299 |
There was a problem hiding this comment.
Such a great code comment 😍
| // Failure to connect is expected and normal here, but users frequently misidentify these errors as the cause of their problems. | ||
|
|
||
| // https://github.com/cypress-io/cypress/issues/19299 | ||
| const isDbusWarning = /Failed to connect to the bus:/ |
There was a problem hiding this comment.
Do you think this is an adequate Regex? I guess it's highly unlikely someone would have "Failed to connect to the bus:" as some other output that would be stripped. Feels a bit less specific than the other ones stripped though.
There was a problem hiding this comment.
This regex is applied pretty narrowly: to the stdout/stderr from electron itself. This doesn't filter any user-generated messages or things coming from our server. I settled on this based on the chromium source code:
if (!connection_) {
LOG(ERROR) << "Failed to connect to the bus: "
<< (dbus_error_is_set(error.get()) ? error.message() : "");
return false;
}
I think it's about as specific as is reasonable here - ideally we won't need to update it as we change electron versions, so the 'technical' stuff up front ([1957:0406/160550.147994:ERROR:bus.cc(392)]) doesn't seem like a good candidate for inclusion.
There was a problem hiding this comment.
@BlueWinds When testing yarn cypress:run locally with yarn docker, there is a new Chromium stderr that looks like it will mislead folks in a similar way:
[371:0406/175035.061253:ERROR:gpu_init.cc(453)] Passthrough is not supported, GL is swiftshader, ANGLE is
Can reproduce by doing yarn docker in the monorepo and running yarn cypress:run --project packages/example.
Also, when I run the latest prod cypress open, I get two log lines:
[343988:0406/135427.824393:WARNING:vaapi_wrapper.cc(586)] VAAPI video acceleration not available for swiftshader
[343988:0406/135427.825018:ERROR:gpu_init.cc(453)] Passthrough is not supported, GL is swiftshader, ANGLE is
Sounds like these could all be related to video acceleration? https://stackoverflow.com/questions/67501093/passthrough-is-not-supported-gl-is-disabled
Feels like these are important to address too, @BlueWinds thoughts?
|
@BlueWinds I am good with the changes, but I'm wondering if this change will satisfy closing the issue it's linked to? Quite a few people have reported issues with crashes & SIGSEGV errors immediately after seeing these dbus errors and this isn't addressing SIGSEGV errors at all. |
I am 100% sure that these errors are not causing the crashes they're seeing. The ticket linked to here contains people reporting half a dozen different bugs - but none of them are actually related to dbus. This is just the last output line, so they assume their problems must be related, and put it all in the same ticket. The hope is, that by hiding these misleading lines, they will open new, more focused, more helpful tickets. Or at the very least, they'll clump their error reports in more useful ways (systems, browsers, versions). It won't address the crashes people are seeing - but a bit 'catch all' ticket ensures that we'll never be able to help them at all. It's for that reason that we need to close it - there's nothing useful in there to help us track down the various real issues people are reporting. |
Yes, I've seen these as well. I don't want to hide them because I don't understand them - I'd want to do another deep-dive into those log lines to see what they mean, if we can fix them, if they're genuinely harmless, etc. before deciding what to do about them. So far I've only done this with the dbus errors, not the GL ones. Would prefer to do that in a separate effort though. |
|
100% agree with your thoughts and reasoning here. Just pointing out if we link this PR as closing this issue, our bot will indicate the issue has been fixed which will be mis-leading since your intentions are close this issue with too many different issues reported and ask people to open new issues with details after upgrading with these error messages filtered out. Might be worth linking this issue, commenting your intentions on the thread when this merges and locking the issue to push towards new issue creation. |
|
I commented on that issue already here, will do it again to clarify once this is merged + the bot comments. |
|
So I originally posted that message. Sorry it caused all the confusion, my original bug report was about You might want to point out to folks that I solved the Thanks for deep diving and hiding the errors, and tell @chrisbreiding I said 👋🏼 |
Yeah, absolutely nothing wrong with your original report! It just got tangled up in a dogpile of other issues, and is now the top (and almost only) result when people google the dbus error, which leads to them to pile in there and connect the two (error + SIGSEV). |
There was a problem hiding this comment.
Yes, I've seen these as well. I don't want to hide them because I don't understand them - I'd want to do another deep-dive into those log lines to see what they mean, if we can fix them, if they're genuinely harmless, etc. before deciding what to do about them. So far I've only done this with the dbus errors, not the GL ones.
Would prefer to do that in a separate effort though.
Totally understand. Is there a follow-up issue or task created for this? I don't want this to get lost, if we don't follow up with that next fix, users will be back in the same confusing place.
#18947. It is marked as 'solved' in the title, but it's an issue specifically related to those errors which users are also finding and identifying as the source of their problems. Not quite as popular as the dbus one, but I'll bring it up for prioritization next. |
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
Silences electron warnings about being unable to connect to dbus. These errors are normal and expected, and do not result in test failures. They are present when running electron inside docker containers 100% of the time.
Additional details
Chromium (which Electron uses) always makes several attempts to connect to the system dbus. This works fine in most desktop environments, but in a docker container, there is no dbus service and Chromium emits several error lines, similar to these:
These warnings are absolutely harmless. Failure to connect to dbus means that electron won't be able to access the user's credential wallet (none exists in a docker container) and won't show up in the system tray (again, none exists). Failure to connect is expected and normal here, but users frequently misidentify these errors as the cause of their problems.
On a more technical level, chromium calls dbus_bus_get(), which is provided by libdbus. This call returns an error if dbus cannot be found or connected to for whatever reason. Chromium handles these errors correctly, internally ceasing to attempt to use the service, but it re-prints the errors to stderr while doing so.
Basically: It tries to connect, gets an error, and aborts. All very normal behavior, which is why no one has tried to 'fix' the problem. Because there is no problem.
How has the user experience changed?
Users will no longer see errors in the log when running Cypress inside docker. Instead of:
they'll get
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?