-
Notifications
You must be signed in to change notification settings - Fork 155
fix for failed to enlarge network buffer error #389
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix for failed to enlarge network buffer error #389
Conversation
@ali-ince I retarget this at 1.4 now based on the discussion we had this morning. |
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.
@ali-ince change looks good. One small question.
"new size is however less than the old size, or because the application " + | ||
"buffer size %s is so big that the application data still cannot fit into the " + | ||
"new network buffer.", curNetSize, netSize, buffer.capacity() ) ); | ||
while ( cipherOut.hasRemaining() ) { |
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.
Would it make sense to limit write attempts here? Could execution get stuck here when unable to write?
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.
Yeah, I also thought on adding a limit but;
If we end up in a position where we could not write anything on a socket, it most probably points a resource starvation on the computer either cpu or network buffers related. In either case the problem lies somewhere else and if we throw some exception here - where theoretically it is not an error - we would break the runtime and eventually become a point of failure.
We also loop on the same condition at line on the same method, so I thought this would also not be a problem.
Does it make sense?
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 found this argument is fair.
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.
Makes sense, and if socket is completely broken exception will be thrown by #channelWrite ()
method.
changed TLSSocketChannel#wrap to agressively do channelWrite upon BUFFER_OVERFLOW. Previously it was failing fast when nothing gets written to the underlying channel which can actually happen on the socket level.