Skip to content

Conversation

@bneradt
Copy link
Contributor

@bneradt bneradt commented Oct 26, 2019

This addresses the issue described by @ema :

#4635

I reproduced the inability to read from the pipe after trafficserver responded to the HUP signal and unlinked the pipe. I next removed the unlink and reproduced the leaked file descriptor Emanuele describes. I then added the close call in the LogFile destructor and the pipe is no longer leaked.

Notice further that I changed the wording in the code comments concerning the meta file for a pipe. Meta files are used only for log rotation and pipes are never rotated since that concept doesn't apply to a pipe. Thus we never associate a meta file with log pipes. This can be seen in the LogFile constructor:

https://github.com/apache/trafficserver/blob/92d4ef1/proxy/logging/LogFile.cc#L74

Notice that BaseLogFile is only created if the LogFile is not a pipe. BaseLogFile is the object that manages the meta file, thus meta files are (correctly) not created for pipes.

@bryancall bryancall added this to the 10.0.0 milestone Oct 28, 2019
@bryancall bryancall merged commit 69a318f into apache:master Oct 28, 2019
@ema
Copy link
Contributor

ema commented Nov 1, 2019

Thanks @bneradt, I've tried your patch on a test host and it seems to work as advertised! I am going to deploy it to some production machines too next week and report back here.

@zwoop
Copy link
Contributor

zwoop commented Nov 4, 2019

Cherry-picked to v9.0.x branch.

@zwoop zwoop modified the milestones: 10.0.0, 9.0.0 Nov 4, 2019
@ema
Copy link
Contributor

ema commented Nov 7, 2019

Thanks @bneradt, I've tried your patch on a test host and it seems to work as advertised! I am going to deploy it to some production machines too next week and report back here.

Works like a charm! Thanks.

@bneradt
Copy link
Contributor Author

bneradt commented Nov 8, 2019

Glad to hear it. Thanks for the update and your detailed initial notes. Your repro steps made working with this issue much easier.

@bneradt bneradt deleted the 4635_pipe_reuse branch November 14, 2019 17:15
@ema
Copy link
Contributor

ema commented Feb 12, 2020

@zwoop can this be cherry-picked to 8.0.x too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants