-
-
Notifications
You must be signed in to change notification settings - Fork 24
Add support for driving BufferedProtocol
instances using sock_recv_into
#60
Conversation
2e48902
to
5aee5a5
Compare
1ffbc38
to
c901d10
Compare
c901d10
to
87a14a5
Compare
…_into` Support for this feature is emulated for the `Pipe{Read,Write}Transport` however, since `glib.IOChannel` does not appear to expose anything similar to the `recvinto` syscall in its APIs. The emulation at least makes it possible to use `BufferedProtocol` instances without causing crashes due to API differences versus the classic `Protocol` interface at least.
87a14a5
to
74a3ec4
Compare
src/gbulb/transports.py
Outdated
assert isinstance(data, bytes) | ||
if data != b"" and data != 0: | ||
if self._alloc_read_buffers: | ||
#FIXME: GLib does not actually expose the equivalent to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This line caused flake8
to cry with error E265, because there is no space between the hash sign and the word FIXME…
(CI passes now, but I had to disable the broken towncrier check.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR (and apologies for the delay in reviewing; I apparently didn't have this repo on my Github Watch list).
I've corrected the problem with the Towncrier configuration; and this PR looks reasonable from a first-pass review. However, in order to be merged, it requires tests for the new functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd feel a lot better if this had tests; but empirical testing shows that it resolves #116, and the perfect is the enemy of the good, so I'll merge it. Thanks for the PR; apologies for taking so long to get to merging it.
Support for this feature is emulated for the
Pipe{Read,Write}Transport
however, sinceglib.IOChannel
does not appear to expose anything similar to therecvinto
syscall in its APIs. The emulation at least makes it possible to useBufferedProtocol
instances without causing crashes due to API differences versus the classicProtocol
interface at least.Refs #58
Fixes #116
PR Checklist: