Skip to content
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

Add flush prior to close on output stream in QueryResource #1441

Merged
merged 1 commit into from
Jul 17, 2015

Conversation

drcrallen
Copy link
Contributor

No description provided.

@@ -183,6 +183,7 @@ public void write(OutputStream outputStream) throws IOException, WebApplicationE
{
// json serializer will always close the yielder
jsonWriter.writeValue(outputStream, yielder);
outputStream.flush(); // Some types of OutputStream suppress flush errors in the .close() method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have some context in which case we see errors getting swallowed or is this purely defensive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purely defensive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, if there is no evidence then why are we fixing it now?
Usual usage pattern of OutputStream I have seen is to just rely on close call unless the usecase really demanded intermediate flushes while you write data and then write more after the flush and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I typed this comment way too fast, let me try again:

Close for many types of output streams will swallow exceptions in flush, leading to a "successful" close when there was really an exception during the final flush.

@fjy
Copy link
Contributor

fjy commented Jul 17, 2015

👍

xvrl added a commit that referenced this pull request Jul 17, 2015
Add flush prior to close on output stream in QueryResource
@xvrl xvrl merged commit 6e8da5d into apache:master Jul 17, 2015
@xvrl xvrl deleted the queryResourceFlusher branch July 17, 2015 23:32
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants