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

TCPConnection.waitForDataAsync #62

Merged
merged 5 commits into from
Feb 26, 2018
Merged

TCPConnection.waitForDataAsync #62

merged 5 commits into from
Feb 26, 2018

Conversation

FraMecca
Copy link
Contributor

I have implemented your suggested changes and I am hoping that we can discuss this PR more if needed.

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);
Copy link
Member

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).

@@ -586,6 +586,41 @@ mixin(tracer);
return m_context.readBuffer.length > 0;
}

enum WaitForDataAsyncStatus {
Copy link
Member

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
@s-ludwig
Copy link
Member

Okay, thanks, that looks good now.

I was thinking whether the runTask is actually such a good idea. Since someone might already have a task running in the background and would just notify it from within the callback, a new task may not always be needed and would just be overhead. For that reason it may be the better solution to just document that no blocking operations should be performed form the callback, since it may be called outside of a task, and to replace the runTask calls by plain callback calls. I'll just quickly add a commit to the PR for that and merge, so that this can still get into the next release.

Also adds a documentation comment to specify the semantics.
@s-ludwig s-ludwig merged commit 6aa5cdd into vibe-d:master Feb 26, 2018
This pull request was closed.
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.

2 participants