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

Handle async cancelled error explicitly #811

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

T-256
Copy link
Contributor

@T-256 T-256 commented Sep 19, 2023

Summary

Discussed in #805

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Copy link
Contributor

@Tunglies Tunglies left a comment

Choose a reason for hiding this comment

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

We'd need to change in both httpcore/_async and httpcore/_sync and ensure them match.

@T-256
Copy link
Contributor Author

T-256 commented Sep 20, 2023

We'd need to change in both httpcore/_async and httpcore/_sync and ensure them match.

As what's said in #806, I think this could be ignored in unasync.py to avoid extra unuseful load. (Task behaviors not defined on threads.)
But still I'm waiting to get proper response from team to choose final implementation for sync part.

@karpetrosyan
Copy link
Member

Is this solution respects such cases?

because the user may catch the keyboardinterrupt in his code and continue to use httpcore.ConnectionPool.

@T-256
Copy link
Contributor Author

T-256 commented Sep 20, 2023

Is this solution respects such cases?

No, #812 will respect.

Actually this PR can expose why current cleanup system is not well implemented.

@karpetrosyan
Copy link
Member

No, #812 will respect.

Do we want to marge this PR when we know it will break programs that handle BaseExceptions like KeyboardInterrupt?

@T-256
Copy link
Contributor Author

T-256 commented Sep 20, 2023

Do we want to marge this PR when we know it will break programs that handle BaseExceptions like KeyboardInterrupt?

Yes. because still some of cleanups doesn't handle BaseException in codebase:

  • except Exception as exc:
    self._connect_failed = True
    raise exc
    elif not self._connection.is_available():

  • except Exception as exc:
    # If we get a network error we should:
    #
    # 1. Save the exception and just raise it immediately on any future reads.
    # (For example, this means that a single read timeout or disconnect will
    # immediately close all pending streams. Without requiring multiple
    # sequential timeouts.)
    # 2. Mark the connection as errored, so that we don't accept any other
    # incoming requests.
    self._read_exception = exc
    self._connection_error = True
    raise exc

  • except Exception as exc: # pragma: nocover
    # If we get a network error we should:
    #
    # 1. Save the exception and just raise it immediately on any future write.
    # (For example, this means that a single write timeout or disconnect will
    # immediately close all pending streams. Without requiring multiple
    # sequential timeouts.)
    # 2. Mark the connection as errored, so that we don't accept any other
    # incoming requests.
    self._write_exception = exc
    self._connection_error = True
    raise exc

  • except Exception as exc:
    self._connect_failed = True
    raise exc
    elif not self._connection.is_available(): # pragma: nocover

  • except Exception as exc: # pragma: nocover
    await self.aclose()
    raise exc
    return AnyIOStream(ssl_stream)

  • except Exception as exc: # pragma: nocover
    await self.aclose()
    raise exc
    return TrioStream(ssl_stream)

  • plus, same things in _sync part.

And also notice few BaseException was introduced in #726, so before that we were not supporting KeyboardInterrupt:

  • First, Before: no error handling, After: BaseException handling.
  • Second, Before: Exception handling, After: BaseException handling.

@T-256 T-256 marked this pull request as ready for review September 20, 2023 14:29
@T-256
Copy link
Contributor Author

T-256 commented Sep 20, 2023

Should we consider convert exception handlings in #811 (comment) to EXCEPTION_OR_CANCELLED?

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.

4 participants