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

Minimise calls to lookupEnv / getenv #166

Closed
Bodigrim opened this issue Apr 20, 2024 · 7 comments
Closed

Minimise calls to lookupEnv / getenv #166

Bodigrim opened this issue Apr 20, 2024 · 7 comments

Comments

@Bodigrim
Copy link
Contributor

I'm trying to mitigate UnkindPartition/tasty#326. The underlying problem is that Setenv is not Thread Safe, so if one thread calls setEnv for any environmental variable and another one calls hSupportsANSI (which executes lookupEnv "TERM" under the hood) the application segfaults.

This is of course not a fault of ansi-terminal at all, but rather a general issue with POSIX standard. Nevertheless it would be helpful to minimize number of calls to lookupEnv. How about adding a top-level lookupEnvTerm as below?

lookupEnvTerm :: Maybe String 
lookupEnvTerm = unsafePerformIO $ lookupEnv "TERM"

And then

hSupportsANSI :: Handle -> IO Bool
hSupportsANSI h = (&&) <$> hIsWritable h <*> hSupportsANSI'
 where
  hSupportsANSI' = (isNotDumb &&) <$> hIsTerminalDevice h
  isNotDumb = lookupEnvTerm /= Just "dumb"

This way lookupEnv "TERM" will be executed at most once, even if hSupportsANSI is called multiple times. IMO this is safe: we do not really care if our application itself at some points modified its environment with setEnv - this could not change the properties of the terminal.

@mpilgrem
Copy link
Collaborator

ansi-terminal has made use of unsafePerformIO in the past (eg in System.Console.ANSI.Windows.Detect.aNSISupport in ansi-terminal-0.11.5), so that is not a barrier as a 'matter of principle'.

However, as hSupportsANSI is 'ancient code' (ansi-terminal-0.6.2, October 2014 - borrowed from a patch to hspec earlier that year), I would like to understand better what is driving the change now. In what circumstances would an application call hSupportsANSI more than once or twice?

@Bodigrim
Copy link
Contributor Author

One could argue that a well-designed application should call supportsANSI only once at the top level (even while I don't think this is the case in practice). But hSupportsANSI should be called on each and every output handle, and there may be multitudes of them.

@mpilgrem
Copy link
Collaborator

I was thinking there were only two handles that needed to be checked: stdout and stderr.

@Bodigrim
Copy link
Contributor Author

Not really, any handle can be a terminal or a pseudo-terminal device. On UNIX any shell session is a terminal device represented as a file in /dev/pts and applications can write to such handle if they wish so.

@mpilgrem
Copy link
Collaborator

@Bodigrim, please can you review #166?

@Bodigrim
Copy link
Contributor Author

Thanks @mpilgrem!

@mpilgrem
Copy link
Collaborator

This is reflected in ansi-terminal-1.1.1, now released.

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

No branches or pull requests

2 participants