Skip to content

Conversation

@Mistuke
Copy link
Contributor

@Mistuke Mistuke commented Feb 20, 2022

Hi,

This PR fixes several issues:

  1. The permissions for the pipes created through createPipe were accidentally swapped. The read end got write permissions while the write part got read permissions in Haskell. The permissions were correct in the underlying OS object but this caused a very weird message when the handles were used. This corrects them so they're in sync.
  2. It documents a requirement for users of createPipe in that since we don't know who's going to use which part of the handles that it's the caller's responsibility to associate the handles with the correct I/O manager. This documents an example on how to do so.
  3. Pipes can be opened in two modes, message and byte, essentially chunk and streaming mode. CreateNamedPipeW defaults to message mode while CreateFile defaulted to byte mode. However the modes need to match. This corrects it by always setting both modes explicitly ensuring that both ends always match. For these pipes we default to chunk instead of streaming mode as things such as iserv require the entire message in order to continue anyway.

This fixes the last issues to enable the new I/O manager with iserv support in GHC.

/cc @bgamari

@Mistuke Mistuke force-pushed the phyx/winio-permissions-and-mode-fixes branch from 3c7f049 to ea44a2f Compare February 20, 2022 09:21
@Mistuke Mistuke force-pushed the phyx/winio-permissions-and-mode-fixes branch from ea44a2f to a101311 Compare February 20, 2022 11:45
Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I only superficially understand this. Does this need a more thorough review from others? And does this warrant an immediate release, or is it acceptable to wait till the next release?

@Mistuke
Copy link
Contributor Author

Mistuke commented Feb 20, 2022

LGTM, though I only superficially understand this. Does this need a more thorough review from others? And does this warrant an immediate release, or is it acceptable to wait till the next release?

Cheers! Thanks for taking a look! Hmm we can wait a day or two see if @bgamari has any comments, but other than that it fixes the last bits of errors in the GHC testsuite when the new I/O manager is enabled by default.

I don't think it needs an immediate release, we're aiming to enable the I/O manager by default in GHC 9.4, so as long as the code is committed I can bump up the submodule in GHC to kick the CI along.

@snoyberg
Copy link
Collaborator

Cool, I'll wait a few days to hear from @bgamari but otherwise merge at the end of the week.

@snoyberg snoyberg merged commit eb45183 into haskell:master Feb 25, 2022
@Mistuke
Copy link
Contributor Author

Mistuke commented Feb 28, 2022

Thanks!

@Mistuke Mistuke deleted the phyx/winio-permissions-and-mode-fixes branch February 28, 2022 21:59
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

Successfully merging this pull request may close these issues.

2 participants