-
Notifications
You must be signed in to change notification settings - Fork 13
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
Remove closeQuietly as close flushes the buffer and throws errors #20
Conversation
I'll try to provide tests if this pr has a chance to get merged. |
@@ -72,7 +72,7 @@ public void copyToMogile(File source, MojiFile destination) throws IOException { | |||
outputStream.flush(); | |||
} finally { | |||
IOUtils.closeQuietly(inputStream); | |||
IOUtils.closeQuietly(outputStream); | |||
outputStream.close();; |
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 find this surprising behaviour as line 72 clearly flushes the buffer. Are you certain this is where the problem lies? How did you find it?
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.
We have an old version of ngnix, which doesn't support chunked
requests and response with a 411 Content length required
, when I call moji.copyToMogile(file, destination)
.
The problem was that neither the exception was thrown nor a useful error log was printed. The issue is related to #21 , which prints
411 Content length required
where as the current behavior ( including this fix ) is throwing an exception that contains
ERR size_verify_error
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.
Can you please paste in the full stack trace thrown with the above code in place? If I understand correctly (it's been a while since I looked at this code) you're saying that the exception you mention in the other PR is being swallowed here. So if we rethrow it instead of swallowing then you should have enough information wherever it ends up and then there won't be a need to log the output which is in the other PR?
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 will post the full stacktrace on monday, when I'm at work again.
you're saying that the exception you mention in the other PR is being swallowed here
So if we rethrow it instead of swallowing then you should have enough information
Actually I thought so as well. However at somepoint the message changes from Content length required
to size_verifiy_error
, which is still puzzling me.
This is the stacktrace, when adding this PR:
|
The root cause is, when file upload to store node is failed, we should not send create_close to tracker node. (Which cause size_verify_error, seems the file is not in the store node) I have some fix about this issue, but still verify it in my production environment. |
@muuki88 the close() method in outputStream is override. size_verify_error is occurred there. |
Cool @hrchu Then we can close this PR and take your fix instead, which bubbles the actual root cause. |
The exception
gets swallowed by the
IOUtils
, which hides upload failures.