High level methods should check status codes and parse errors

[domenic]

See #59 and #113. In short, things like this:

var client = knox.createClient({
    key: 'ABCGARBAGE',
    secret: 'UTTERLYSHOULDNEVERWORK',
    bucket: 'mybucket'
});

var putReq = client.putFile(source, dest, function(e, res){
    assert(e !== null);
});

should give e !== null, with a useful description in the error message, instead of relying on the user to check res.statusCode.

Things to be aware of:

• This will require XML parsing, but we have an xml2js dependency now to take care of that.
• There are more success codes than 200; in particular anything in the 2xx range is success, and you could argue that anything in the 3xx range is also (but see putFile/putStream API should handle HTTP 307 redirects from AWS S3 #66).
• Dynode's implementation

[boutell]

Points to consider re: 3xx status codes:

• Amazon won't send that redirect until you've already uploaded lots and lots of data. This can't be resolved transparently in the lower level APIs without buffering all uploads somewhere all the time just in case of a 3xx status code. So automatically fixing these without developer intervention is not a viable option.
• The putFile API could automatically fix these, since it can rewind the file. However that encourages people to upload big things twice all the time with no knowledge that they are doing so, all because they didn't specify the region option, something they can easily do. So I don't think putFile and friends should automatically handle redirects either.

I think 3xx status codes should set err to an object (err should be truthy). A clean way to recognize that that object represents a redirect could be provided, but in all of the cases I'm aware of, the 3xx codes are best resolved by specifying a region in the first place. In fact the error should probably say "Redirect (did you forget to set the region option?)"

[kof]

I have migrated from 0.5.0 to 0.8.3 and upload stopped to work. I was getting ECONNRESET errors from uncaughtException handler with almost empty stack trace.

After lots of debugging:

1. Issue is only reproducible on region eu-west-1. We are using some us buckets and some eu. Weird thing was that us buckets worked, eu not.
2. The reason ECONNRESET was thrown is because the error happened AFTER knox has removed the error listener.
3. I would recognize the error immediately if I could see the error message sent by amazon:

"PermanentRedirectThe bucket you are attempting to access must be addressed using the specified endpoint. Please send all future requests to this endpoint.B73833C012C5C994test-irland.skim.comrK5HZux4Pvafr5lnIl9WFV21CTYOWjRQq5I9cOoAK5jrnaVMPOjK8qsv4ZFG5pSftest-irland.skim.com.s3.amazonaws.com"

I could see this error after I started to listen on 'data' event on response object.

client.putBuffer(new Buffer('aaa'), 'test.txt', {'Content-Type': 'text/plain'}, function(err, res) {
    console.error('Done', err);
    res.on('data', function(data) {
        console.log(data.toString('utf8'));
    });
}).on('error', function(err) {
    console.error('Error', err);
})

I think the ONLY thing knox should do is to parse the response from amazon and to pass the error in high level functions or emit error on lower level once

[domenic]

@kof Thanks for this. I'm curious what we did that caused the regression. My suspicion is 13ed446. Before that commit, if a response occurs but then an error also occurs, we would call back twice: first with the response or with an error, second with the ECONNRESET error. This is related to nodejs/node-v0.x-archive#5510.

So I'm not sure what the best path would be for this. Maybe we should ignore any errors after the first one?

---

There's the separate issue of parsing the error and displaying it, yeah. We should really do that.

[kof]

I think this is the issue about parsing the response.

1. Do we need to parse the response in case of anything else than error?
   If not, we can speedup it, to not do xml parsing for EVERY response.
   /error/gi.test(body), if true -> parse xml, build error, pass to callback
2. After we have got a response, do we need to expect any IMPORTANT errors which SHOULD be forwardeded ?
   This is the problem of convertig emitter style api to callback style

[kof]

About regression:
The actual regression is that now I need to pass region option, on 0.5.0 it has worked without. (for eu-west-1)

[domenic]

/error/gi.test(body), if true -> parse xml, build error, pass to callback

Sounds like a good idea, agreed.

This is the problem of convertig emitter style api to callback style

Yes :(. I am not sure how to handle it.

I guess we could swallow the error, under the assumption that if two things go wrong, you just need to know that things went wrong, not that both things went wrong. I can't see a much better solution, assuming our existing approach of forcing users to not only check the err parameter but also attach an 'error' listener is not acceptable. (And to me that strategy seems more "right" but also way less developer friendly, so...)

The actual regression is that now I need to pass region option, on 0.5.0 it has worked without. (for eu-west-1)

Opened #188 to investigate this, thanks.

[aseemk]

Just wanted to chime in that at a high level (rimshot), I'd love to see 4xx and 5xx responses automatically treated as errors also. =) Right now, wherever we call Knox methods, we have to check that manually, which isn't great.

Two quick comments/q's based on the thread so far:

1. Why treat 3xx as an error? Amazon returns temporary redirects sometimes, e.g. if they're having DNS issues or if one of their datacenters goes down. (And it happens w/ bucket creates; see issue putFile/putStream API should handle HTTP 307 redirects from AWS S3 #66.)
2. Why detect whether to parse XML based on a regex of the body? (Which seems leaky, as valid files on S3 may have the word 'error' in them...) / Why wouldn't it be as easy as switching on the status code? 2xx/3xx vs. 4xx/5xx.

I appreciate the work that goes into great libs like this. Thanks again.

[kof]

1.Because in this case its actually an error for the user, but statusCode is 301

Example code
create the client without region option, create a bucket in eu-west-1 before running it

client.putBuffer(new Buffer('aaa'), 'test.txt', {'Content-Type': 'text/plain'}, function(err, res) {
    console.error('Done', err);
    res.on('data', function(data) {
        console.log(data.toString('utf8'));
    });
}).on('error', function(err) {
    console.error('Error', err);
})

Response

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>PermanentRedirect</Code><Message>The bucket you are attempting to access must be addressed using the specified endpoint. Please send all future requests to this endpoint.</Message><RequestId>BFD3C28C12904D02</RequestId><Bucket>test-irland.skim.com</Bucket><HostId>apNbihqml6A2KsKVyrS0mH9m5Dmowalruxy9yRXyscYLibYPesb4ts8UY6DNsyfO</HostId><Endpoint>test-irland.skim.com.s3.amazonaws.com</Endpoint></Error>

2.My idea was to stick to the 'error' in the response body and ignore status code, because there are cases like that one, where 301 is actually an error for the user. But yes regexp for xml is dirty, however to avoid conflicts regexp could match <Error><Code>. This would also cover any cases we don't know right now where status code is potentially not from errors area, but the body says it is an error.

But if we do know exactly that amazon always send 3xx and 5xx codes where error code could be found, instead of doing regexp check, we can check status code and if its in the 3x or 5x area, parse xml and generate an error if "Error" is inside.

[aseemk]

Yes, that's true, but Amazon also sends back <Error> XML for temporary 307 redirects -- which are not predictable or within the client's control.

Looking at the list of errors:

http://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html

These 301 Permanent and 307 Temporary redirects are the only non-4xx/5xx error status codes. And the thing about the 301 case is that it's still a workable response -- Knox, like any HTTP client, just needs to follow the redirect.

So how about in the 301 or 307 case, simply raising a redirect event or similar, and logging a warning to the console by default, so the developer is made aware, but not fail in either case.

[kof]

So the current state:

1. if there is a 3xx status code and endpoint defined in the response body - try again with the correct endpoint setting
2. if there is 4xx or 5xx status code - parse response and generate a user error

[aseemk]

Sounds solid to me. For the 3xx case, you also don't even need to parse the body: the endpoint will be the value of the Location header if it's a redirect.

[domenic]

We should read and parse the bodies of errors, and probably even successes, to avoid having to hand people raw response objects with all their associated hazards. See e.g. #197.

[aseemk]

Good call, for responses that send Amazon success/error bodies.

For successful PUTs and DELETEs, there's no body, right? (It's a 204, right?) And obv for successful GETs, we shouldn't read the body ourselves.

[domenic]

Right, for gets we definitely shouldn't. For PUTs and DELETEs, we should be sure to close off the response one way or another. I'm not entirely sure what that entails---maybe just doing res.resume() and then doing nothing with it? But mainly, avoid the case in #197.

[f0rmiga]

How can I make distinction from successful deleted object and not found object? Both responses from S3 assume the code 204.

[wachunga]

Just encountered this today: list swallows the 403 statusCode with no error and returns this as data:

{ 
  Code: 'SignatureDoesNotMatch',
  Message: 'The request signature we calculated does not match the signature you provided. Check your key and signing method.',
  ...
  Contents: []
}

[boutell]

Given the lack of commits in the last 9 months and the lack of response to
#263, I wonder if this project is semi-abandoned...

On Wed, Oct 7, 2015 at 8:21 PM, David Hirtle notifications@github.com
wrote:

Just encountered this today: list swallows the 403 statusCode with no
error and returns this as data:

{
Code: 'SignatureDoesNotMatch',
Message: 'The request signature we calculated does not match the signature you provided. Check your key and signing method.',
...
Contents: []
}

—
Reply to this email directly or view it on GitHub
#114 (comment).

*THOMAS BOUTELL, *DEV & OPS
P'UNK AVENUE | (215) 755-1330 | punkave.com