Open domenic opened 11 years ago
Points to consider re: 3xx status codes:
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?)"
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:
"PermanentRedirect
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
@kof Thanks for this. I'm curious what we did that caused the regression. My suspicion is 13ed4464ad784057e0d4d6b11a859944bd7f80d9. 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 https://github.com/joyent/node/issues/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.
I think this is the issue about parsing the response.
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)
/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.
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:
I appreciate the work that goes into great libs like this. Thanks again.
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.
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.
So the current state:
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.
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.
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.
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.
How can I make distinction from successful deleted object and not found object? Both responses from S3 assume the code 204.
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: []
}
Given the lack of commits in the last 9 months and the lack of response to
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 https://github.com/Automattic/knox/issues/114#issuecomment-146377117.
THOMAS BOUTELL, DEV & OPS P'UNK AVENUE | (215) 755-1330 | punkave.com
See #59 and #113. In short, things like this:
should give
e !== null
, with a useful description in the error message, instead of relying on the user to checkres.statusCode
.Things to be aware of: