-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
fix: broken unclosed files on windows #1139
base: gh-windows
Are you sure you want to change the base?
Conversation
Unfortunately this is a breaking change. We cannot update the public waf interface. |
I thought so already. Please let me know if I can do anything to help getting this merged to v4. |
This looks good to me. Just remove the closer from the interface and keep
the others.
…On Sat, Aug 24, 2024 at 7:45 PM jabdr ***@***.***> wrote:
I thought so already. Please let me know if I can do anything to help
getting this merged to v4.
—
Reply to this email directly, view it on GitHub
<#1139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAS2NXLCDPK7WWOCZILZTDBFLAVCNFSM6AAAAABNBXVCLWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBYGQ3DSNJQGE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Actually giving a second thought we should only close those closeables we
open so let's carefully review the cases.
On Sun, Aug 25, 2024 at 10:25 AM José Carlos Chávez <
***@***.***> wrote:
… This looks good to me. Just remove the closer from the interface and keep
the others.
On Sat, Aug 24, 2024 at 7:45 PM jabdr ***@***.***> wrote:
> I thought so already. Please let me know if I can do anything to help
> getting this merged to v4.
>
> —
> Reply to this email directly, view it on GitHub
> <#1139 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAXOYAS2NXLCDPK7WWOCZILZTDBFLAVCNFSM6AAAAABNBXVCLWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBYGQ3DSNJQGE>
> .
> You are receiving this because your review was requested.Message ID:
> ***@***.***>
>
|
I can do that. Should I create a seperate PR for v4 that includes the close on the public interface?
Do you mean the debuglog Close? Let me think about it. |
I removed the changes to debuglog itself. You are correct, it should not close the debug file handle, as it only receives the io.Writer from WAF. Instead I choose to add a field to the internal WAF struct. So the file creator/opener is also closing the file in the end. I removed the public WAF interface change from this PR, but I recommend to add it to the next major version. |
Please rebase your PR on top of |
This patch fixes tests and log file behaviour on windows. The big problem here is, that windows does not allow to delete files with open file descriptors.
To solve this problem I had to implement the following changes:
This was originally intended as a small fix, but now it requires an API change. Technically, it is not required to Close the log files on linux, but I think it is still a good best practice to not rely on GC alone.
I removed the os.Remove calls from testing/auditlog_test.go and replaced them with Close, as go test already cleans them up, but only if they are properly closed (on windows).