-
Notifications
You must be signed in to change notification settings - Fork 85
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
Ensure callback is called once in doWhilst with S3 operations #2168
Conversation
Hello williamlardier,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
The trigger seem to be aws/aws-sdk-js#1678 ... causing the callback to be called twice, which is a problem in these loops. Issue: ZENKO-4925
b7402e8
to
b88d962
Compare
@@ -596,8 +597,9 @@ class ReplicationUtility { | |||
Key: key, | |||
VersionId: versionId, | |||
}, (err, data) => { | |||
const cbOnce = jsutil.once(callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I understand correctly, this would happen if/because the headObject callback is invoked twice : both for the actual response and a socket timeout which happens at about the same time.
→ it seems to me a simpler fix would be just:
callback => this.s3.headObject({
Bucket: bucketName,
Key: key,
VersionId: versionId,
}, jsutil.once((err, data) => {
[...]
})
However, I am wondering if there may not be a real underlying issue : like the socket timeout (on client side, i.e. test) being shorter than the timeout we have in zenko.... and maybe this could explain the (other) issues we have with these tests, what do you think?
Can we check the timeout (or infirm this hypothesis), and/or should we ignore retryable AWS errors (retry after a timeout) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure it was a timeout but worth reviewing that indeed, let me check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current client-side timeout seem to be infinite, so I don't think we are affected by the timeout from client side:
const scalityS3Client = new S3({
...
// disable node sdk retries and timeout to prevent InvalidPart
// and SocketHangUp errors. If retries are allowed, sdk will send
// another request after first request has already deleted parts,
// causing InvalidPart. Meanwhile, if request takes too long to finish,
// sdk will create SocketHangUp error before response.
maxRetries: 0,
httpOptions: { timeout: 0 },
});
Will be merged with the upcoming Zenko release - keeping this open till then. |
The trigger seem to be
aws/aws-sdk-js#1678
... causing the callback to be called twice, which is a problem in these loops.
Issue: ZENKO-4925
Rationale of the change in the ticket comments.