Skip to content

Commit

Permalink
reader: fix map handling on PyPy
Browse files Browse the repository at this point in the history
Previously, maps were handled in a following way:
  1. `key` parsed
  2. `{key: None}` stored in the resulting value
--- New iteration of parsing ---
  3. `value` parsed
  4. `dict.popitem()` called to retrieve the parsed `key`
  5. `{key: value}` stored

This was hackish but worked fine with a dict. However, with a new mode
of handling maps as lists of tuples, it started working incorrectly with
PyPy, where assignment of `value` was always ignored.

This commit introduces a new variable `pendingObject` inside
`libvalkey_ReaderObject` that represents the pending key to be stored in
a map item. Hence, the new way of parsing a map item is now the
following:

  1. `key` parsed and stored inside `self->pendingObject`
--- New teration of parsing ---
  2. `value` parsed
  3. `key` taken from `self->pendingObject`
  4. `{key: value}` stored

Signed-off-by: Mikhail Koviazin <[email protected]>
  • Loading branch information
mkmkme committed Jul 19, 2024
1 parent ff330ac commit 1e3be1f
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 22 deletions.
61 changes: 39 additions & 22 deletions src/reader.c
Original file line number Diff line number Diff line change
Expand Up @@ -73,23 +73,25 @@ PyTypeObject libvalkey_ReaderType = {
Reader_new, /*tp_new */
};

static int tryParentize_impl(const valkeyReadTask *task, PyObject *obj, PyObject *parent) {
static int tryParentize_impl(const valkeyReadTask *task, PyObject *obj,
PyObject *parent, libvalkey_ReaderObject *self) {
switch (task->parent->type) {
case VALKEY_REPLY_MAP:
if (task->idx % 2 == 0) {
/* Set a temporary item to save the object as a key. */
if (PyDict_SetItem(parent, obj, Py_None) < 0) {
/* Save the object as a key. */
self->pendingObject = obj;
} else {
if (self->pendingObject == NULL) {
Py_DECREF(obj);
return -1;
}
} else {
/* Pop the temporary item and set proper key and value. */
PyObject *last_item = PyObject_CallMethod(parent, "popitem", NULL);
PyObject *last_key = PyTuple_GetItem(last_item, 0);
if (PyDict_SetItem(parent, last_key, obj) < 0) {
if (PyDict_SetItem(parent, self->pendingObject, obj) < 0) {
Py_DECREF(obj);
Py_DECREF(self->pendingObject);
self->pendingObject = NULL;
return -1;
}
self->pendingObject = NULL;
}
break;
case VALKEY_REPLY_SET:
Expand All @@ -101,31 +103,45 @@ static int tryParentize_impl(const valkeyReadTask *task, PyObject *obj, PyObject
break;
default:
assert(PyList_CheckExact(parent));
PyList_SET_ITEM(parent, task->idx, obj);
if (PyList_SetItem(parent, task->idx, obj) < 0) {
Py_DECREF(obj);
return -1;
}
}
return 0;
}

static int tryParentize_ListOnly(const valkeyReadTask *task, PyObject *obj, PyObject *parent) {
static int tryParentize_ListOnly(const valkeyReadTask *task, PyObject *obj,
PyObject *parent, libvalkey_ReaderObject *self) {
switch (task->parent->type) {
case VALKEY_REPLY_MAP:
if (task->idx % 2 == 0) {
/* Set a temporary item to save the object as a key. */
PyObject *t = PyTuple_New(2);
PyTuple_SET_ITEM(t, 0, obj);
PyTuple_SET_ITEM(t, 1, Py_None);
PyList_Append(parent, t);
Py_DECREF(t);
self->pendingObject = PyTuple_New(2);
if (self->pendingObject == NULL) {
Py_DECREF(obj);
return -1;
}
PyTuple_SET_ITEM(self->pendingObject, 0, obj);
} else {
/* Pop the temporary item and set proper key and value. */
PyObject *last_item = PyObject_CallMethod(parent, "pop", NULL);
PyTuple_SET_ITEM(last_item, 1, obj);
PyList_Append(parent, last_item);
if (self->pendingObject == NULL) {
Py_DECREF(obj);
return -1;
}
PyTuple_SET_ITEM(self->pendingObject, 1, obj);
int res = PyList_Append(parent, self->pendingObject);
Py_DECREF(self->pendingObject);
self->pendingObject = NULL;
if (res < 0)
return -1;
}
break;
default:
assert(PyList_CheckExact(parent));
PyList_SET_ITEM(parent, task->idx, obj);
if (PyList_SetItem(parent, task->idx, obj) < 0) {
Py_DECREF(obj);
return -1;
}
}
return 0;
}
Expand All @@ -135,8 +151,8 @@ static void *tryParentize(const valkeyReadTask *task, PyObject *obj) {
if (task && task->parent) {
PyObject *parent = (PyObject*)task->parent->obj;
int res = self->listOnly
? tryParentize_ListOnly(task, obj, parent)
: tryParentize_impl(task, obj, parent);
? tryParentize_ListOnly(task, obj, parent, self)
: tryParentize_impl(task, obj, parent, self);
if (res < 0)
return NULL;
}
Expand Down Expand Up @@ -389,6 +405,7 @@ static PyObject *Reader_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
self->shouldDecode = 1;
self->protocolErrorClass = LIBVALKEY_STATE->VkErr_ProtocolError;
self->replyErrorClass = LIBVALKEY_STATE->VkErr_ReplyError;
self->pendingObject = NULL;
self->listOnly = 0;
Py_INCREF(self->protocolErrorClass);
Py_INCREF(self->replyErrorClass);
Expand Down
2 changes: 2 additions & 0 deletions src/reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ typedef struct {
PyObject *notEnoughDataObject;
int listOnly;

PyObject *pendingObject;

/* Stores error object in between incomplete calls to #gets, in order to
* only set the error once a full reply has been read. Otherwise, the
* reader could get in an inconsistent state. */
Expand Down

0 comments on commit 1e3be1f

Please sign in to comment.