Repository navigation
Deprecate socket sendall method in favor of SocketStream.send_all #291
Description
I looked at the high level async socket.sendall() trio 0.1.0 code and realized nothing would stop a user from calling sendall() from 2 tasks on the same socket, and having the individual lowlevel send()'s being intertwined, which is probably not what the user wants (ie fragmented/mashed sendall() data ordering).
Some options are:
- write some concurrency protection, raise an error
- write some documentation saying sendall() is not atomic
- make sendall atomic (via locking/parking lots, etc because I don't think we can safely disable checkpoints)
Activity
njsmith commentedon Aug 18, 2017
The high-level
SocketStreamwrapper does do this check. (Though that code is a bit confusing -- it may saylock, but it actually does raise an error if multiple tasks try to call it concurrently.) So doestrio.hazmat.wait_socket_writable, which is the low-level primitive thatsendcalls when it needs to block. So the situation where this could go wrong is when someone is using the socketsendallmethod directly (hopefully rare once the high-level API is released), and a somewhat tricky race happens like:sendallcallssend, which does a partialsend(meaning the output buffer is full)sendallgets scheduled immediately afterwards, before Task A can run again, and itssendsucceeds, because those extra few milliseconds were enough for the kernel to drain the send buffersendallcallssendagain.This is probably rare but I bet it can happen in real life.
The high-level layer can't just delegate worrying about
sendallto the socket layer, because it needs to also catch conflicts betweensend_allandwait_send_all_might_not_block. I think the only way to make this all work sensibly without tons of duplicate work would be if we made "I am using this resource please make everyone else error out" explicitly part of the lower-level APIs, either at the socket or I/O loop level. And this is tricky because for UDP I don't think you necessarily want this checking onsendto, because theresendtoactually is atomic. Well, but right now you can't necessarily just blithely callsendtofrom multiple tasks on UDP and assume it will work, because I think it can returnEWOULDBLOCKin some weird cases, and then we'll callwait_socket_writableand that will raiseResourceBusyErrorif multiple tasks do it at the same time. So you would need asendto_nowaitto really take advantage of this. But maybe we should havesendto_nowait; I don't think we want to rule that out, anyway.So I think I've argued myself into saying that for now, we should add a note to the documentation warning that if you try calling the socket-level
sendallsimultaneously from multiple tasks then you get undefined behavior (and also have you considered using our nice higher-level APIs?).I guess there's even an argument for deprecating and removing the socket-level
sendall, on the grounds that it's a convenience API and if you want convenience than ourStreamlayer is the thing to use. Sort of like how we don't bother providing atrio.socket.create_connectionemulation ofsocket.create_connection, because people should just use the much fanciertrio.open_tcp_stream.(Is there ever any reason to call
sock.sendall(...)on a non-SOCK_STREAMsocket?)njsmith commentedon Aug 18, 2017
I looked at this a little. Definitely not
SOCK_DGRAMorSOCK_RAW. ForSOCK_SEQPACKETI... can't tell, exactly. The docs are terrible. But I think the only thing that could even potentially make sense would besendallwithMSG_MOREfollowed by a call tosendwithoutMSG_MORE, but this seems like a weird way to do things. I don't think you can get a partial result back fromsend.njsmith commentedon Aug 18, 2017
On even more investigation: POSIX says, regarding
SOCK_SEQPACKET: "A record can be sent using one or more output operations and received using one or more input operations, but a single operation never transfers parts of more than one record. Record boundaries are visible to the receiver via the MSG_EOR flag in the received message flags returned by the recvmsg() function." Buuuut apparently Linux'sAF_UNIXSOCK_SEQPACKETimplementation just totally ignores all of this except for "a single operation never transfers parts of more than one record". Specifically, there doesn't seem to be any way to send a message over multiple calls (MSG_MOREdoes nothing), and there doesn't seem to be a way to receive one either (it doesn't actually giveMSG_EOR, and it's like UDP: you get one call to receive each message, and you either get the whole thing or a truncated version, that's it). So yeah, no reason to usesendallhere at all.Document that the socket sendall method is not task-safe
njsmith commentedon Aug 18, 2017
#292 for the documentation part.
[-]Think about socket/stream sendall() high level concurrency protection[/-][+]Deprecate socket sendall method in favor of SocketStream.send_all[/+]Deprecate socket sendall method