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

[Process] fix locking of pipe files on Windows #28689

Merged
merged 1 commit into from Oct 10, 2018
Merged

[Process] fix locking of pipe files on Windows #28689

merged 1 commit into from Oct 10, 2018

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28655
License MIT
Doc PR -

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Oct 2, 2018
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 2, 2018
Closed
@nicolas-grekas nicolas-grekas changed the title [Process] fix skipping already used temporary indexes [Process] fix locking of pipe files on Windows Oct 4, 2018
@nicolas-grekas
Copy link
Member Author

@SailorMax patch updated, can you try the new one please?

@SailorMax
Copy link

With new patch all my processes has exitCode = 1 and did not work at all. Can't write stdout and stderr to locked files?

@nicolas-grekas
Copy link
Member Author

Thanks, can you try again please? It should be good now (at least appveyor is green.)

@SailorMax
Copy link

Yes, latest patch work as expected in my case.
Thank you!

@SailorMax
Copy link

But after work temp files remain in the temp-directory. Is it feature? :)

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Oct 5, 2018 •
edited

It is :) Actually there is no way around: unlinking the file would create race conditions.
The compromise here is to use incremental numbers for files so that there should be only a limited number of them to keep in the tmp dir.
We used to use random files to remove any locks, but the outcome was worse: there is no way to delete these tmp files 100% of the time, leading to the tmp dir being filled with empty files on real-world apps. A much worse outcome.

@SailorMax
Copy link

For me - not a problem :)

@fabpot
Copy link
Member

fabpot commented Oct 10, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit d64bd3b into symfony:2.8 Oct 10, 2018
fabpot added a commit that referenced this pull request Oct 10, 2018
@fabpot
bug #28689 [Process] fix locking of pipe files on Windows (nicolas-gr…
9fdc64b 
…ekas)

This PR was merged into the 2.8 branch.

Discussion
----------

[Process] fix locking of pipe files on Windows

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28655
| License       | MIT
| Doc PR        | -

Commits
-------

d64bd3b [Process] fix locking of pipe files on Windows
This was referenced Nov 3, 2018
Merged
Merged
Merged
This was referenced Mar 12, 2021
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Bug Process Status: Reviewed
Projects
None yet
Milestone
2.8
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nicolas-grekas @SailorMax @fabpot @carsonbot