[JENKINS-37098] don’t rely on ReopenableFileOutputStream #2490
Conversation
ndeloof
added
the
needs-more-reviews
|
@reviewbybees |
| @@ -37,6 +37,7 @@ | |||
| * and then keep writing. | |||
| * | |||
| * @author Kohsuke Kawaguchi | |||
| * @deprecated due to risk for file leak | |||
| */ | |||
| public class ReopenableFileOutputStream extends OutputStream { | |||
There was a problem hiding this comment.
Also needs @Deprecated annotation
|
@ndeloof Needs a JIRA reference. won't this just cause an IOException on windows as |
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
|
@jtnord what you describe is what happens on OSX : as slave dir has been deleted, stream fails to get re-opened. This PR is just a workaround to limit impact, actual fix would require to ensure every component has logged what it needs before closing the log. Anyway the last step is to delete the log, so not sure this would benefit end-user :P |
oleg-nenashev
added
the
needs-jira-issue
|
From what I see the only difference between old and new classes is |
|
@oleg-nenashev code indeed is very similar but ReopenableFileOutputStream intent is wrong : letting you re-open the stream without control on who is responsible to close it is source for file leaks. So I prefer to mark this as deprecated to later remote it, vs create a common ancestor that won't make much sense after removal. |
ndeloof
removed
the
needs-jira-issue
recampbell
changed the title
| @@ -37,8 +37,9 @@ | |||
| * and then keep writing. | |||
| * | |||
| * @author Kohsuke Kawaguchi | |||
| * @deprecated due to risk for file leak | |||
There was a problem hiding this comment.
Link to the alternative (RewindableFileOutputStream I guess)?
There was a problem hiding this comment.
This isn't an alternative, but a subset of what ReopenableFileOutputStream intended to do.
|
🐝 I think there is still an issue in the core that causes someone to attempt to log after a log has been closed() - this is currently causes ephemeral cloud slaves to blow up when they are removed on windows, and this should fix it - but as we can now delete the directory (whereas before the delete of the directory would fail as there where open handles - now the callback that wrote to the log will throw an IOException when attempting to write to the log that it has infact closed.) So please raise a JIRA for a follow up to cover this case. |
| @@ -137,7 +136,7 @@ | |||
|
|
|||
| public SlaveComputer(Slave slave) { | |||
| super(slave); | |||
| this.log = new ReopenableRotatingFileOutputStream(getLogFile(),10); | |||
| this.log = new RewindableFileOutputStream(getLogFile()); | |||
There was a problem hiding this comment.
do we not still want some rolling here?
There was a problem hiding this comment.
yes indeed, I forgot this one
|
LGTM 🐝 |
ReopenableFileOutputStream introduces file-leaks (detected on windows) when channel onClose writes on already closed log, and breaks when computer tries to delete the slave’s log directory.
… copied another.
ReopenableFileOutputStream introduces file-leaks (detected on windows) when channel onClose writes on already closed log, and breaks when computer tries to delete the slave’s log directory.
see JENKINS-37098