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

LTS v21.12 Deprecations #2306

Merged
merged 28 commits into from Dec 23, 2021
Merged

LTS v21.12 Deprecations #2306

merged 28 commits into from Dec 23, 2021

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Nov 7, 2021

There are a number of things we need to remove in the next release:

  • abort
  • CompositionView
  • StreamingHTTPResponse
  • Sanic and Blueprint to use __slots__
  • Config.load_env

Most of these are straightforward. The open question is re: StreamingHTTPResponse. More particularly, the two convenience methods we had for instantiating it: response.stream and response.file_stream.

While the release notes and warning have made it clear that StreamingHTTPResponse would be removed, we also said that the convenience methods would stick around for some time. Therefore: we must provide backwards compat in v21.12.

I have accomplished this with a new ResponseStream object which is a fairly lightweight and is meant to be a compat layer. What we really need to do is to decide the fate of stream and file_stream.

I propose the following:

  1. Deprecate stream and mark it for removal in v22.6.
  2. Continue with support for file_stream in the form proposed in this PR.

There really is no longer a utility in stream. Even if we do not mark it for removal, I would advocate for removing it from the docs and leaving it as undocumented in the User Guide since it is no longer the preferred method for handling these. On the other hand, file_stream does have utility separate from StreamingHTTPResonse, so I think we should carry that one forward.

Other thoughts? Opinions?

@ahopkins ahopkins requested a review from Tronic November 7, 2021 21:34
@Tronic
Copy link
Member

Tronic commented Nov 14, 2021

I am happy to see old streaming removed in all forms, even if it takes some time. The return file_stream(...) thing is quite useful, so we should support that based on the new streaming system. I cannot quite recall what the current state of this is.

Also, there actually might not be much use for supporting file and file_stream return values separately, they ought both be streaming when necessary, e.g. based on file size.

@ahopkins
Copy link
Member Author

Also, there actually might not be much use for supporting file and file_stream return values separately, they ought both be streaming when necessary, e.g. based on file size.

@Tronic another point worth mentioning is the discussion that started in Discord re: sync v async file reads. While async like what is currently in Sanic is non-blocking, it is also much slower. Combining file and file_stream does make sense, but we should also consider what method to use for the reading since smaller files will actually be better served with a little bit of blocking using regular open.

@Tronic
Copy link
Member

Tronic commented Nov 15, 2021

Combining file and file_stream does make sense, but we should also consider what method to use for the reading since smaller files will actually be better served with a little bit of blocking using regular open.

Strictly all blocking disk I/O is best avoided when the server is running. Unfortunately even the simplest disk access can take 10+ seconds when SSDs are doing their reorganization, when btrfs is doing its, or when mechanical drives are spinning up. Throughput needs to be optimized elsewhere, e.g. reading all small static files (if known not to change on disk, or if there is a background thread that monitors for changes) to RAM and serving from there (probably needs to be done by the application or at the very least an extension, as it is hard to see how Sanic core could offer that).

@ahopkins ahopkins mentioned this pull request Dec 19, 2021
@ahopkins ahopkins marked this pull request as ready for review December 23, 2021 06:54
@ahopkins ahopkins requested review from a team as code owners December 23, 2021 06:54
@ahopkins ahopkins changed the title Begin deprecations for v21.12 LTS v21.12 Deprecations Dec 23, 2021
@ahopkins ahopkins added the needs review the PR appears ready but requires a review label Dec 23, 2021
prryplatypus
prryplatypus previously approved these changes Dec 23, 2021
examples/blueprints.py Outdated Show resolved Hide resolved
examples/delayed_response.py Show resolved Hide resolved
examples/modify_header_example.py Outdated Show resolved Hide resolved
examples/redirect_example.py Show resolved Hide resolved
examples/request_stream/server.py Outdated Show resolved Hide resolved
tests/test_deprecation.py Show resolved Hide resolved
tests/test_exceptions.py Show resolved Hide resolved
tests/test_exceptions_handler.py Outdated Show resolved Hide resolved
tests/test_unix_socket.py Show resolved Hide resolved
tests/test_versioning.py Show resolved Hide resolved
Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
ahopkins and others added 10 commits December 23, 2021 22:58
Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
This is actually going to become *VERY* important if we change the multiprocessing from `fork` to `spawn` in next version.

Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
@ahopkins ahopkins merged commit 8c07e38 into main Dec 23, 2021
@ahopkins ahopkins deleted the deprecations branch December 23, 2021 22:30
mildbyte added a commit to mildbyte/strawberry that referenced this pull request Dec 28, 2021
This is for compatibility with newly-released Sanic 21.12 which removed the deprecated `abort()` in sanic-org/sanic#2306. Before that, it was a simple wrapper around `raise SanicException` (https://github.com/sanic-org/sanic/blob/523db190a732177eda5a641768667173ba2e2452/sanic/exceptions.py#L262-L265), so this change makes it explicit and removes the dependency on `abort()`.
mildbyte added a commit to mildbyte/strawberry that referenced this pull request Jan 3, 2022
This is for compatibility with newly-released Sanic 21.12 which removed the deprecated `abort()` in sanic-org/sanic#2306. Before that, it was a simple wrapper around `raise SanicException` (https://github.com/sanic-org/sanic/blob/523db190a732177eda5a641768667173ba2e2452/sanic/exceptions.py#L262-L265), so this change makes it explicit and removes the dependency on `abort()`.
patrick91 added a commit to strawberry-graphql/strawberry that referenced this pull request Jan 5, 2022
* Replace `abort()` with `raise SanicException`

This is for compatibility with newly-released Sanic 21.12 which removed the deprecated `abort()` in sanic-org/sanic#2306. Before that, it was a simple wrapper around `raise SanicException` (https://github.com/sanic-org/sanic/blob/523db190a732177eda5a641768667173ba2e2452/sanic/exceptions.py#L262-L265), so this change makes it explicit and removes the dependency on `abort()`.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add RELEASE.md

* Bump sanic from 21.9.3 to 21.12.0

Bumps [sanic](https://github.com/sanic-org/sanic) from 21.9.3 to 21.12.0.
- [Release notes](https://github.com/sanic-org/sanic/releases)
- [Changelog](https://github.com/sanic-org/sanic/blob/main/CHANGELOG.rst)
- [Commits](sanic-org/sanic@v21.9.3...v21.12.0)

---
updated-dependencies:
- dependency-name: sanic
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update sanic-testing to 0.8.2

(0.7.0 uses the old `app.test_client` attribute which has been deprecated in
Sanic 21.12).

* Update tests

* Skip sanic tests on windows

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Patrick Arminio <patrick.arminio@gmail.com>
ChihweiLHBird pushed a commit to ChihweiLHBird/sanic that referenced this pull request Jun 1, 2022
Co-authored-by: Néstor Pérez <25409753+prryplatypus@users.noreply.github.com>
@ahopkins
Copy link
Member Author

ahopkins commented Jul 3, 2022

This pull request has been mentioned on Sanic Community Discussion. There might be relevant details there:

https://community.sanicframework.org/t/v22-6-cant-not-use-sanic-auto-register-true/1035/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review the PR appears ready but requires a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants