Support machines with multiple NICs #576
Conversation
Co-authored-by: Michael Graeb <[email protected]>
source/s3_client.c
Outdated
| goto cleanup; | ||
| } | ||
| network_interface_names[i] = aws_byte_cursor_from_pyunicode(str_obj); | ||
| Py_DECREF(str_obj); |
There was a problem hiding this comment.
😬 now that I'm thinking about it, this isn't 1000% safe either
if (in theory) the sequence wasn't a tuple or list but something with a custom __getitem__() that returned a new object, then when we decref it here, it would get cleaned up, and our char * would be pointing at discarded memory
I looked at how PySequence_Fast_GET_ITEM() could get away with a borrowed reference, and it does it by ensuring a list or tuple was passed it, and if it was something else, it creates a list instead so it can be sure the items live at least as long as whatever's returned (see code)
anyway blugh ugh ugh ugh ugh I realize this bug exists elsewhere in our C-bindings
maybe keep these items alive until cleanup, similar to the array of cursors. Like:
- declare them before any gotos
struct aws_byte_cursor *network_interface_name_cursors = NULL; PyObject **network_interface_name_pyobjects = NULL; aws_mem_calloc()them at the same timenetwork_interface_name_pyobjects[i] = PySequence_GetItem(network_interface_names_py, i); /* New reference */- then in cleanup, loop through and Py_XDECREF() them all before aws_mem_release of the tmp array
There was a problem hiding this comment.
I'm not TRYING to make your life miserable. I'm just like "oh hey PySequence exists, let's use it" ... "uh oh"
Another option is doing like PySequence_Fast .. and turning it into a list (if necessary) before processing it. In python we could just be like:
if network_interface_names is not None:
# ensure this is a list, so it's simpler to process in C
if not isinstance(network_interface_names, list):
network_interface_names = list(network_interface_names)
There was a problem hiding this comment.
Thanks, I have updated it to the second option.
Co-authored-by: Michael Graeb <[email protected]>
| @@ -235,6 +242,7 @@ def __init__( | |||
| throughput_target_gbps, | |||
| float) or throughput_target_gbps is None | |||
| assert isinstance(enable_s3express, bool) or enable_s3express is None | |||
| assert isinstance(network_interface_names, Sequence) or network_interface_names is None | |||
There was a problem hiding this comment.
I added the assert again because we will be converting it to a list.
Description of changes:
Adds an option to take a list of interface names and distribute connections across multiple NIC.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.