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.

Sign up for GitHub

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

Jump to bottom

windows: Fix restarts using the raw_exec driver #5864

Merged
merged 7 commits into from
Jul 2, 2019
Merged

windows: Fix restarts using the raw_exec driver #5864

merged 7 commits into from
Jul 2, 2019

Conversation

endocrimes
Copy link
Contributor

@endocrimes endocrimes commented Jun 20, 2019 •
edited
Loading

fixes #5803

This currently uses a personal fork of go-winio to add an api that can safely open multiple listeners for a named pipe.

It fixes local restarts on Windows by:

  • No longer trying to recreate named pipes that already exist
  • Correctly closing connections when stopping a Read

@nickethier
Copy link
Member

Instead of repackaging we could use govendor fetch github.com/microsoft/go-winio::github.com/endocrimes/go-winio to keep the same paths. This seems like something we should try and upstream if possible so we don't have to maintain a fork.

@endocrimes endocrimes force-pushed the dani/win-pipe-cleaner branch from 483bf81 to f3652ee Compare June 21, 2019 14:31
notnoop
notnoop suggested changes Jun 27, 2019
View reviewed changes
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

+1 to @nickethier suggestion of govendor fetch origin support, we use in other places and it's handy.

Also, let's add a regression test for this case.

client/logmon/logmon.go Outdated Show resolved Hide resolved
client/lib/fifo/fifo_test.go Outdated Show resolved Hide resolved
client/lib/fifo/fifo_test.go Outdated Show resolved Hide resolved
client/lib/fifo/fifo_unix.go Show resolved Hide resolved
endocrimes and others added 3 commits June 28, 2019 13:41
@endocrimes
logmon: Refactor fifo access for windows safety
efda81c 
On unix platforms, it is safe to re-open fifo's for reading after the
first creation if the file is already a fifo, however this is not
possible on windows where this triggers a permissions error on the
socket path, as you cannot recreate it.

We can't transparently handle this in the CreateAndRead handle, because
the Access Is Denied error is too generic to reliably be an IO error.
Instead, we add an explict API for opening a reader to an existing FIFO,
and check to see if the fifo already exists inside the calling package
(e.g logmon)
@endocrimes
vendor: Use dani fork of go-winio
76f72fe
@endocrimes
fifo: Require that fifos do not exist for create
e6daf3b 
Although this operation is safe on linux, it is not safe on Windows when
using the named pipe interface. To provide a ~reasonable common api
abstraction, here we switch to returning File exists errors on the unix
api.
@endocrimes endocrimes force-pushed the dani/win-pipe-cleaner branch 12 times, most recently from 5ef76e3 to c697df9 Compare June 28, 2019 13:58
@endocrimes endocrimes force-pushed the dani/win-pipe-cleaner branch 2 times, most recently from d36439f to 36bf4df Compare June 28, 2019 14:39
notnoop
notnoop reviewed Jul 1, 2019
View reviewed changes
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

The code lgtm - but I have a question about the lock used in Close.

client/logmon/logmon_test.go Outdated Show resolved Hide resolved
client/lib/fifo/fifo_windows.go Show resolved Hide resolved
@endocrimes endocrimes force-pushed the dani/win-pipe-cleaner branch from 467af87 to 8148466 Compare July 1, 2019 12:14
notnoop
notnoop approved these changes Jul 1, 2019
View reviewed changes
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

couple of questions but merge away as you see fit.

client/lib/fifo/fifo_windows.go Outdated Show resolved Hide resolved
@endocrimes endocrimes merged commit 7968f79 into master Jul 2, 2019
@endocrimes endocrimes deleted the dani/win-pipe-cleaner branch July 2, 2019 11:59
@ilyaiqoqo ilyaiqoqo mentioned this pull request Jul 18, 2019
Closed
@shoenig shoenig mentioned this pull request Jan 12, 2022
Open
@github-actions
Copy link

github-actions bot commented Feb 7, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@notnoop notnoop notnoop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Task failing with logmon failed to create fifo for extracting logs
4 participants
@endocrimes @nickethier @notnoop @preetapan