falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.53k stars 945 forks source link

Unintended sharing of `MultipartParseOptions._DEFAULT_HANDLERS` between instances #2293

Closed vytas7 closed 2 months ago

vytas7 commented 2 months ago

I think we should initialize media_handlers in MultipartParseOptions.__init__() with a copy MultipartParseOptions._DEFAULT_HANDLERS, not just assign reference:

>>> from falcon.media.multipart import MultipartParseOptions
>>> mo1 = MultipartParseOptions()
>>> mo2 = MultipartParseOptions()
>>> sorted(mo1.media_handlers)
['application/json', 'application/x-www-form-urlencoded']
>>> mo1.media_handlers.pop('application/x-www-form-urlencoded')
<falcon.media.urlencoded.URLEncodedFormHandler object at 0x7dce2c618f50>
>>> mo1.media_handlers
{'application/json': <falcon.media.json.JSONHandler object at 0x7dce2c7fa990>}
>>> mo2.media_handlers
{'application/json': <falcon.media.json.JSONHandler object at 0x7dce2c7fa990>}
>>> MultipartParseOptions._DEFAULT_HANDLERS
{'application/json': <falcon.media.json.JSONHandler object at 0x7dce2c7fa990>}
VibhinnS commented 2 months ago

Hi! I tried fixing the issue using deepcopy method and I'm seeing desired results in the shell. But while writing the tests, the media_handlers keys of both mo1 and mo2 are getting passed as reference. The shell behavior does not replicate there.

I tried modifying _DEFAULT_HANDLERS in handlers.py as well.

Could you please give me a hint where am I going wrong?

vytas7 commented 2 months ago

Hi @VibhinnS! And thanks for looking into this!

It is hard for me to guess exactly what didn't work in your tests vs the interpreter shell without seeing the code. Could you maybe create a Draft PR or share a code Gist? (Or, in a pinch, just paste some code here in the comment thread.)

Edit: and as to general advice how to solve this, I was thinking to make _DEFAULT_HANDLERS a tuple of tuples or just a dict, and then initialize a new instance of MediaHandlers from that data instead of assignment, but I haven't tested this or looked closer at it more than that.

VibhinnS commented 2 months ago

```python class MultipartParseOptions: _DEFAULT_HANDLERS = None

# Unchanged code...

def __init__(self):
    self.default_charset = 'utf-8'
    self.max_body_part_buffer_size = 1024 * 1024
    self.max_body_part_count = 64
    self.max_body_part_headers_size = 8192
    self.media_handlers = copy.deepcopy(self._DEFAULT_HANDLERS)

```

Added copy.deepcopy() to self._DEFAULT_HANDLERS to ensure media_handlers is a deep copy.

shell op - ```python

from falcon.media.multipart import MultipartParseOptions mo1 = MultipartParseOptions() mo2 = MultipartParseOptions() sorted(mo1.media_handlers) ['application/json', 'application/x-www-form-urlencoded']

mo1.media_handlers.pop('application/x-www-form-urlencoded') <falcon.media.urlencoded.URLEncodedFormHandler object at 0x000001C6A1317530>

mo1.media_handlers {'application/json': <falcon.media.json.JSONHandler object at 0x000001C6A13174D0>}

mo2.media_handlers {'application/json': <falcon.media.json.JSONHandler object at 0x000001C6A35D58E0>, 'application/x-www-form-urlencoded': <falcon.media.urlencoded.URLEncodedFormHandler object at 0x000001C6A35D51F0>}

MultipartParseOptions._DEFAULT_HANDLERS {'application/json': <falcon.media.json.JSONHandler object at 0x000001C6A2F8C5C0>, 'application/x-www-form-urlencoded': <falcon.media.urlencoded.URLEncodedFormHandler object at 0x000001C6A3509550>} ```

Wrote a basic test file test_multipart.py in the tests directory:

```python from falcon.media.multipart import MultipartParseOptions

mo1 = MultipartParseOptions() mo2 = MultipartParseOptions()

sorted(mo1.media_handlers) mo1.media_handlers.pop('application/x-www-form-urlencoded')

print(mo1.media_handlers) print("\n") print(mo2.media_handlers) print("\n") print(MultipartParseOptions._DEFAULT_HANDLERS) ```

However, the test returns unexpected results:

```python {'application/json': <falcon.media.json.JSONHandler object at 0x000002006DE227B0>}

{'application/json': <falcon.media.json.JSONHandler object at 0x000002006DE227B0>}

{'application/json': <falcon.media.json.JSONHandler object at 0x000002006DE227B0>} ```

vytas7 commented 2 months ago

That deepcopy is maybe a slight overkill, but it should do the job for prototyping, I see nothing wrong there.

Maybe you are still using another version of Falcon when running your test?

Try in your venv:

FALCON_DISABLE_CYTHON=1 pip install -e . pytest
pytest tests/test_your_new_file.py
CaselIT commented 2 months ago

I agree that a simple copy is likely enough, since the handles instance are not expected to be modified inplace

VibhinnS commented 2 months ago

Aite. Let me try with the copy method alone

vytas7 commented 2 months ago

Hi again @VibhinnS, just checking if you are still working on this issue, and do you need any help?

myusko commented 2 months ago

Hey folks,

If no one doesn't work on this one, I'll take a look.

vytas7 commented 2 months ago

Whoa it's been a while @myusko! πŸ‘‹ πŸ‡ΊπŸ‡¦

Happy to have your help on this one.