-
Notifications
You must be signed in to change notification settings - Fork 801
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
[SCB-484] Servlet rest support download #750
Conversation
bbabccb
to
3a7e22b
Compare
action.run(); | ||
} | ||
} catch (IOException e) { | ||
currentBufferCount--; |
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.
Here is no sychronized. If we just need to make sure currentBufferCount is automatic, we don't need to use the synchroized to do this job.
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.
not just protect currentBufferCount, we must protect close/drain logic.
/** | ||
* for pump from a readStream | ||
*/ | ||
public class OutputStreamToWriteStream implements WriteStream<Buffer>, AsyncCloseable<Void> { |
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.
Too much synchronized in this class, if we can make sure there is only one thread handle this write stream, that could make our life much easier.
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 readStream and writeStream both are vertx eventloop stream, then it's only one thread
but if one of readStream and writeStream is wrapped from a blocked stream(inputStream/outputStream), then it's multiple thread
so we must use synchronized logic just like vertx asyncFile do.
did not provide download integration test, but already test locally
because upload did not support for servlet yet,
but it's not good to delete upload test case.