webargs: Race conditions for parallel requests due to cache
I just noticed that something in webargs or marshmallow isn’t thread-safe. Take this minimal example"
import time
from flask import Flask, jsonify, request
from marshmallow.fields import Field
from webargs.flaskparser import use_kwargs
app = Flask(__name__)
class MyField(Field):
def _serialize(self, value, attr, obj):
return value
def _deserialize(self, value, attr, data):
print 'deserialize', request.json, value
time.sleep(0.25)
return value
@app.route('/test', methods=('POST',))
@use_kwargs({
'value': MyField(),
})
def test(value):
time.sleep(1)
return jsonify(webargs_result=value, original_data=request.json['value'])
Run it with threading enabled:
$ FLASK_APP=webargsrace.py flask run -p 8080 --with-threads
Now send two requests in parallel, with different values:
$ http post http://127.0.0.1:8080/test 'value=foo' & ; http post http://127.0.0.1:8080/test 'value=bar' &
The output from these two requests is:
{
"original_data": "bar",
"webargs_result": "bar"
}
{
"original_data": "foo",
"webargs_result": "bar"
}
Clearly not what one would have expected! 💣
The output of the print statement showing the request data and what the field receives confirms the issue:
deserialize {u'value': u'bar'} bar
deserialize {u'value': u'foo'} bar
Tested with the latest marshmallow/webargs from PyPI, and also the marshmallow3 rc (marshmallow==3.0.0rc4, webargs==5.1.2).
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 1
- Comments: 17 (17 by maintainers)
Commits related to this issue
- Remove the Parser cache Because the cache is no longer used field-by-field to fetch data, there's significantly less value in keeping it. Combined with the fact that each parser instantiation was alr... — committed to sirosen/webargs by sirosen 4 years ago
- Remove the Parser cache Because the cache is no longer used field-by-field to fetch data, there's significantly less value in keeping it. Combined with the fact that each parser instantiation was alr... — committed to sirosen/webargs by sirosen 4 years ago
- Remove the Parser cache Because the cache is no longer used field-by-field to fetch data, there's significantly less value in keeping it. Combined with the fact that each parser instantiation was alr... — committed to sirosen/webargs by sirosen 4 years ago
- Remove the Parser cache Because the cache is no longer used field-by-field to fetch data, there's significantly less value in keeping it. Combined with the fact that each parser instantiation was alr... — committed to sirosen/webargs by sirosen 4 years ago
I’m looking into fixing it right now
The patch is released in 5.1.3. I’ve also requested a CVE ID and will report on Tidelift once that’s done.
We can discuss refactoring the solution in https://github.com/marshmallow-code/webargs/issues/374 .
Thank you @ThiefMaster for the quick response on this.
I think this is caused by the fact that
self._cacheon theParseris not thread-safe.