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

fix read loop with shutdown #210

Merged
merged 2 commits into from
Jun 2, 2021
Merged

fix read loop with shutdown #210

merged 2 commits into from
Jun 2, 2021

Conversation

dpatti
Copy link
Collaborator

@dpatti dpatti commented May 22, 2021

When you shutdown the connection and we close the reader, a surprising
thing happens under certain conditions: the read loop wakes up, asks for
the next read operation, and is told to yield. It tries to yield, and
then the state machine raises -- you shouldn't be yielding if the
connection is closed!

The reason for this is our explicit return of [`Yield] in
_final_read_operation_for. As mentioned in my lengthy comment, I think
the real issue here is that if we want to circumvent Reader.next, we
have to make sure we're not yielding when the reader cannot take any
more input anyway.

I thought of fixing this in a few ways, like adding explicit yielding
and continuing mechanisms to the reader at request boundaries, which is
something that has come up in the past as being a bit tricky to reason
about. I also considered just tossing this check at the top of
next_read_operation, but it feels like the kind of thing that I'd
later ask "why is this necessary?" I want so badly for the reading and
writing behaviors to be parallel, so I did something that is meant to
feel less permanent.

This test also fails before the refactor-request-queue branch. I found
it curious, but then realized that Server_connection.shutdown is not
used in either of our runtimes, so it's not too surprising that we
didn't have decent coverage of it.

Note on testing

One of the solutions I tried was to change the peristent_connection
branch to also check closed state. But then I realized that you could
have a closed reader but still want to process more connections, so you
wouldn't want to simply shut the whole thing down. I added a missing
test that will fail if you do the wrong thing, now.

When you shutdown the connection and we close the reader, a surprising
thing happens under certain conditions: the read loop wakes up, asks for
the next read operation, and is told to yield. It tries to yield, and
then the state machine raises -- you shouldn't be yielding if the
connection is closed!

The reason for this is our explicit return of [`Yield] in
`_final_read_operation_for`. As mentioned in my lengthy comment, I think
the real issue here is that if we want to circumvent `Reader.next`, we
have to make sure we're not yielding when the reader cannot take any
more input anyway.

I thought of fixing this in a few ways, like adding explicit yielding
and continuing mechanisms to the reader at request boundaries, which is
something that has come up in the past as being a bit tricky to reason
about. I also considered just tossing this check at the top of
`next_read_operation`, but it feels like the kind of thing that I'd
later ask "why is this necessary?" I want so badly for the reading and
writing behaviors to be parallel, so I did something that is meant to
feel less permanent.

This test also fails before the `refactor-request-queue` branch. I found
it curious, but then realized that `Server_connection.shutdown` is not
used in either of our runtimes, so it's not too surprising that we
didn't have decent coverage of it.

Note on testing
---------------

One of the solutions I tried was to change the `peristent_connection`
branch to also check closed state. But then I realized that you could
have a closed reader but still want to process more connections, so you
wouldn't want to simply shut the whole thing down. I added a missing
test that will fail if you do the wrong thing, now.
@dpatti dpatti requested a review from seliopou May 22, 2021 19:33
@dpatti
Copy link
Collaborator Author

dpatti commented May 22, 2021

The test came from investigation in #209

@dpatti dpatti merged commit 6dc5737 into master Jun 2, 2021
@dpatti dpatti deleted the test-server-shutdown branch June 2, 2021 21:25
@dpatti dpatti restored the test-server-shutdown branch June 2, 2021 22:46
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