-
Notifications
You must be signed in to change notification settings - Fork 605
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
Improved socket implementation to recycle buffer in Socket.readXX ope… #809
Conversation
@mpilquist this complains about the binary compatibility, but hence the change is not in api, I still believe its ok to publish it in 0.9.3 any thoughts? |
@pchlupacek Yeah that's fine -- can you add an exclusion to |
getBufferOf(max) flatMap { buff => | ||
readChunk(buff, timeout.map(_.toMillis).getOrElse(0l)) flatMap { | ||
case (read, _) => | ||
if (read < 0) F.pure(None) |
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.
The readSemaphore
isn't incremented here -- is that okay?
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.
thanks for spotting this
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.
but actually, hence this signals eof, i think we won't hit it,neverthelss I gonna fix it for corectness
if (readBytes < 0 || buff.remaining() <= 0) F.pure(Some(Chunk.bytes(buff.array(), 0, buff.position()))) | ||
else go(buff,(timeoutMs - took) max 0) | ||
def read0(max:Int, timeout:Option[FiniteDuration]):F[Option[Chunk[Byte]]] = { | ||
readSemaphore.decrement >> |
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.
Do we need any error handling here? E.g., if one of these tasks throws, we still want to increment the semaphore, right?
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.
Well I think if you fail the read, there is no way to recover it anyhow, so my thought was just to let it be. Do you see there is a way that read will fail, and there is chance to recover it with next read?
@mpilquist made modifications as you have suggested. Not sure about this mimaBinaryIssueFilters where they are? |
@pchlupacek I took care of the mima issue: b02fd92 |
@mpilquist thanks |
…rations