Skip to content

Replace shared global _empty_stream with separate instances#1340

Merged
davidism merged 1 commit intopallets:masterfrom
chromakode:remove-shared-empty-stream
Aug 4, 2018
Merged

Replace shared global _empty_stream with separate instances#1340
davidism merged 1 commit intopallets:masterfrom
chromakode:remove-shared-empty-stream

Conversation

@chromakode
Copy link
Contributor

@chromakode chromakode commented Aug 3, 2018

The use of a shared _empty_stream instance can cause subtle failures when a test or process closes the shared stream.

For example, if a test initializes a request with a file upload FileStorage with a falsy stream argument, the FileStorage will use the shared _empty_stream:

self.stream = stream or _empty_stream

When the request is torn down, it will close the shared _empty_stream:

value.close()

self.stream.close()

Subsequently, if a test environment is created with no specified input_stream, it will use the closed _empty_stream:

input_stream = _empty_stream

Now calls to request.get_data() and other operations on the stream will fail.

In the case where we discovered this behavior at Patreon, two unrelated tests were causing a failure: one which initialized an FileStorage with _empty_stream, and one which called request.get_data().

I believe that replacing this shared instance with individually created BytesIO buffers would reduce the chances of such spooky shared global bugs without creating much overhead. What do you think?

@davidism
Copy link
Member

davidism commented Aug 3, 2018

Makes sense. Would you add a changelog entry?

@chromakode chromakode force-pushed the remove-shared-empty-stream branch 2 times, most recently from 6e3e3e4 to 0d23117 Compare August 3, 2018 20:51
If a test closed the _empty_stream, subsequent requests could reuse this
closed stream, causing request.get_data() and other methods to fail.
@chromakode chromakode force-pushed the remove-shared-empty-stream branch from 0d23117 to baf2051 Compare August 3, 2018 21:02
@chromakode
Copy link
Contributor Author

Thanks @davidism. I've amended the commit with a changelog entry and the commit is now passing lint.

@davidism davidism merged commit 8c4e6b5 into pallets:master Aug 4, 2018
@davidism
Copy link
Member

davidism commented Aug 4, 2018

Thanks!

@chromakode chromakode deleted the remove-shared-empty-stream branch August 4, 2018 00:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants