Bugfix/zenko 1197/s3 data leak #578
Conversation
Hello vrancurel,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
| // this means the underlying socket has been closed | ||
| log.debug('Client closed socket while streaming', | ||
| { method: 'put', key, filePath, | ||
| error: 'socket closed' }); |
There was a problem hiding this comment.
error should be an error object with message attribute to not mess up indexation. I suggest either wrapping it in an error object or calling the attribute name errorMessage
There was a problem hiding this comment.
Is it fine if I remove the error: field ? because there is no error in fact.
There was a problem hiding this comment.
That works too :)
| @@ -199,6 +199,16 @@ class DataFileStore { | |||
| return cbOnce(errors.InternalError.customizeDescription( | |||
| `read stream error: ${err.code}`)); | |||
| }); | |||
| dataStream.on('close', () => { | |||
There was a problem hiding this comment.
Does the close event never ever trigger in a legitimate end of stream?
There was a problem hiding this comment.
no, only in case of a close() during a stream processing.
vrancurel
force-pushed
the
bugfix/ZENKO-1197/s3-data-leak
branch
from
012e50b
to
4f70a74
Compare
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
Follow integration pull requests if you would like to be notified of |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
When receiving this callback, sometimes the socket is already closed (e.g. upon RST) but sometimes we have to cloud it ourselves.
When the underlying socket of the dataStream is closed this is not considered as a stream error. So we have to hook the event and do the cleanup by ourselves
History mismatchMerge commit #4e522a4d78a0efd75ac1c53a78f66119c9d9a673 on the integration branch It is likely due to a rebase of the branch Please use the |
|
@bert-e reset |
Reset completeI have successfully deleted this pull request's integration branches. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
Follow integration pull requests if you would like to be notified of |
History mismatchMerge commit #4749bb7a6cf62ff668b39ad6dd9656f484128b83 on the integration branch It is likely due to a rebase of the branch Please use the |
vrancurel
force-pushed
the
bugfix/ZENKO-1197/s3-data-leak
branch
from
4f70a74
to
3dee6e2
Compare
|
@bert-e reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: wait |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
Follow integration pull requests if you would like to be notified of |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue ZENKO-1197. Goodbye vrancurel. |
Fixes a leak occurring when cloudserver abruptly closes the s3-data socket. The root-cause of this socket hang-up is for the moment unclear, but is probably due to some overload.
The main source of the leak is that we don't listen to the 'close' event on the dataStream. When receiving this event we have to destroy the fileStream and delete the filePath by ourselves because it is not managed by the pipe. Without this, the file descriptor is never closed, the incomplete file is never deleted, leaking disk space, and leaking VFS cache which is critical in e.g. kubernetes pods.
Another problem is that we don't destroy the socket upon receiving of a clientError event in the lib/network/http/server.js. Indeed the socket is not necessarily closed automatically by the underlying layers (e.g. upon receiving a RST it seems that the underlying socket is closed, but not in some cases when it is a normal FIN/ACK termination handshake). If we don't explicitly destroy the socket sometimes the 'close' event is not emitted to the dataStream.