-
Notifications
You must be signed in to change notification settings - Fork 46
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
TCPConnection.waitForDataAsync #62
Conversation
source/vibe/core/net.d
Outdated
auto tm = setTimer(timeout, { eventDriver.sockets.cancelRead(m_socket); | ||
runTask(read_callback, false); }); | ||
eventDriver.sockets.read(m_socket, m_context.readBuffer.peekDst(), IOMode.once, | ||
(sock, st, nb) { tm.stop(); if(st != IOStatus.ok) runTask(read_callback, false); |
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 two lines from above
assert(m_context.readBuffer.length == 0);
m_context.readBuffer.putN(nbytes);``
should be executed right after the tm.stop()
, so that the read data is actually committed to the read buffer. The if
condition would then ideally be m_context.readBuffer.length > 0
instead of st == IOStatus.ok
, because partial data could have been read before the error was raised (not a realistic case, since IOMode.once
is used, but handling it this way should be slightly more robust).
source/vibe/core/net.d
Outdated
@@ -586,6 +586,41 @@ mixin(tracer); | |||
return m_context.readBuffer.length > 0; | |||
} | |||
|
|||
enum WaitForDataAsyncStatus { |
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.
To reduce the amount of text to type when using this from the outside, I'd move this outside below the TCPConnection
class.
assert and stronger condition on TCPConnection.waitForDataAsync
Okay, thanks, that looks good now. I was thinking whether the |
Also adds a documentation comment to specify the semantics.
I have implemented your suggested changes and I am hoping that we can discuss this PR more if needed.