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 exit_backend() race condition #77

Merged
merged 3 commits into from
Dec 22, 2022
Merged

Conversation

inimino
Copy link
Contributor

@inimino inimino commented Dec 15, 2022

Adds an exception type that was missing from exit_backend() (found by API fuzz testing by tests/0040*).

@inimino inimino requested a review from ryttry December 15, 2022 00:57
@inimino
Copy link
Contributor Author

inimino commented Dec 15, 2022

I pushed another change to this same branch since I was here, this takes away the printing of error_msg to stdout from the Python SDK. Given a typical usage, I think this would be more confusing than helpful (I was confused by it myself already).

There might be some more Pythonic way to alert users to errors rather than requiring them to check error_msg, but that definitely wasn't it.

# We allowing passing this error as it is known.
except http.client.RemoteDisconnected:
# see docstring above
except (http.client.RemoteDisconnected, http.client.IncompleteRead):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When, or in which test cases under which scenarios, does IncompleteRead occur?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the remote end goes away before reading any bytes, it will be RemoteDisconnected, after reading HTTP headers (incuding Content-Length) it will be IncompleteRead if fewer bytes are then sent.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which test cases, at which point in the test suite, or at what test conditions, are you meeting IncompleteRead that you regard as expected and therefore not raising its exception?

I think it's important we raise the exception, save areas that we know it will happen in because of known reasons that we accept, because we should almost always expect not having IncompleteRead while testing.

This ensures unexpected errors don't get shielded from the radar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice that we are in the context of the exit_backend() method here?

This function exits the backend. Note that the API call will usually fail
because the backend exits before returning a HTTP response so we suppress
the error.

The user sends a request to the backend that says "end the process and free all resources" and the backend frees all resources instantly. The backend will not spend any more resources on any outstanding HTTP requests, including this one. The user would never possibly care about or want to see these errors in any possible scenario, so we suppress them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I'm going to merge this as the other PR is also landing today. I'm not sure my explanation was sufficient here, but let's find a time to chat about this!

src/watchful/client.py Outdated Show resolved Hide resolved
@ryttry
Copy link
Collaborator

ryttry commented Dec 16, 2022

Adds an exception type that was missing from exit_backend() (found by API fuzz testing by tests/0040*).

Where would I be able to find this test?

@inimino
Copy link
Contributor Author

inimino commented Dec 16, 2022

Adds an exception type that was missing from exit_backend() (found by API fuzz testing by tests/0040*).

Where would I be able to find this test?

https://github.com/Watchfulio/watchful/pull/1823

@inimino
Copy link
Contributor Author

inimino commented Dec 16, 2022

To observe this you could start the backend and exit it repeatedly a few thousand times. Most of the time you would get the "RemoteDisconnected" error but a few times per thousand you might get the "IncompleteRead" error, because the process teardown happened to occur in the middle of the bytes of the HTTP response being sent over the wire. (This is what I meant by calling it a race condition).

@inimino
Copy link
Contributor Author

inimino commented Dec 16, 2022

e.g. you could run something like this:

import etc etc

while True:
    backend_for_tests()
    client.exit_backend()

Remove the try/except from the exit_backend() function and run the above to see what I'm talking about.

@inimino inimino merged commit b0c98df into main Dec 22, 2022
@inimino inimino deleted the inimino/summary-invariant-test branch December 22, 2022 20:31
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.

3 participants