webbrowser::open is non-blocking on macOS but blocking on linux

[samuela]

The title is pretty self-explanatory. I'm on macOS 10.14.2 and Ubuntu 18.10.

This is weird to me because $BROWSER is not set and xdg-open is also non-blocking. I'm not really sure why but in any case it would be nice to have consistent behavior across platforms.

[samuela]

Note: This only applies when I don't have Firefox already open. If I have a Firefox window open already, then it appears to be non-blocking again...

[amodm]

Indeed, it seems to be the case. From the looks of it, in Linux, calling Command::output() blocks when its stdout/err is set to Stdio::pipe() instead of Stdio::inherit(). This is likely happening because the browser that was opened is still holding a reference to the pipe fd that must've been created. Don't know why this doesn't happen on macOS.

But if inherit were to be used, the point of return an Output is meaningless, as it won't be captured anyways. In hindsight, return ExitStatus was probably better, instead of Output.

For the time being, I think I'll keep API compatibility by returning Output, but internally switch to ExitStatus

[amodm]

@samuela can you confirm this works?

[samuela]

@amodm Ok, so this kinda works AFAICT. I just upgraded to version 0.5.1. It still works the same way in macOS. It now works on the ubuntu VM (same version and setup as in the first comment). But it still seems to be blocking when SSHing (with X11 forwarding) into the ubuntu box from the macOS machine. Admittedly, I didn't test this case when reporting the issue initially so I'm not sure whether or not this is a regression.

[samuela]

@amodm Thanks for your great turnaround time btw!

[amodm]

That's super weird. I just tested on Ubuntu 18.04 (server) and macOS (client, using XQuartz), , and seems to work exactly as it's supposed to locally. Ubuntu to Ubuntu works as well.

[amodm]

Can you detail out the config you're trying?

[amodm]

@samuela if you do find time to share the config that can help reproduce the SSH issue, let's track it in a separate issue.

[amodm]

Documenting here that new consistent behaviour is now enforced. See Consistent Behaviour section for details.

Relevant commit: 614cacf