fix(jadepy): avoid blocking large serial reads#292
Conversation
|
Please do not submit AI patches without noting you have done so. Also please provide some kind of rationale for this change. |
|
Does this one also try to solve the connection problem with newer cbor2 6.X.X versions? I noticed that my jade does not connect anymore after Arch released python-cbor2 6.1.1-1 last week (which is probably unsupported intentionally as there where breaking changes and it is excluded here: Line 41 in cdcebe8 I was able to hack an LLM based workaround to get the new cbor version connected again: @@ -64,6 +64,15 @@
else:
return data
+
+def _cbor_load(fp):
+ try:
+ return cbor.load(fp, read_size=1)
+ except TypeError as e:
+ if 'read_size' not in str(e):
+ raise
+ return cbor.load(fp)
+
@@ -1858,6 +1867,12 @@
traceback.print_tb(tb)
self.disconnect(exc_type is not None)
+ def readable(self):
+ return True
+
+ def seekable(self):
+ return False
+
@@ -2103,8 +2118,8 @@
"""
while True:
# 'self' is sufficiently 'file-like' to act as a load source.
- # Throws EOFError on end of stream/timeout/lost-connection etc.
- message = cbor.load(self)
+ # Throws EOFError/CBORDecodeEOF on end of stream/timeout/lost-connection etc.
+ message = _cbor_load(self)
@@ -2158,7 +2173,7 @@
while True:
try:
return self.read_cbor_message()
- except EOFError as e:
+ except (EOFError, getattr(cbor, 'CBORDecodeEOF', EOFError)) as e:
if not long_timeout:
raise
Would be cool if we could get an updated jade library that also supports newer cbor2 version. We also had issues with changes in 5.8.0 (#286). I could totally understand if Blockstream refuses to do so and requires to use vendored cbor2 with the supported version. |
|
Hi @1-21gigasats Unfortunately the cbor2 upstream seems recently prone to breakage (undoubtedly because as the author notes, people did not given timely feedback on the latest release candidate(s)). I think arch aggressively moving to new global versions of a package that is repeatedly breaking back-compat is not smart packaging policy, especially given these errors manifest at runtime not build-time. My view of the exception handling is that deriving from ValueError would be nicer for downstream projects, and I hope that they change their mind on this. However for your issue, if we explicitly catch CBORDecodeEOF we should convert that to EOFError early (i.e. inside _cbor_load in your changes) and not carry knowledge of it outside of any wrapper function. That said, the removal of read_size and requirement to add more stream methods needs investigation of the upstream changes before they can be included. |
|
Thanks for your feedback @jgriffiths - I actually noticed this issue early while the new cbor2 version was still in Arch Extra Testing Repo but I was unsure how to handle it properly. If I had this reported as an arch packaging issue the package maintainer would probably point me to either cbor2 or Jade to fix the issue. Cbor2 is required by only 4 packages which Electrum is one of and was only added because I asked for. I think we cannot expect from arch package maintainer to check for this kind of edge case. I offered to send him one DIY-Jade for testing, but he did not came back to this. With four parties involved (Arch Packager, Cbor2, Jade and Electrum) it feels some kind of hard to address an issue like this properly. Should I have directly opened an issue at cbor2? I would have expected that they send be back here to fix it downstream. If we look at how often cbor2 broke in the past maybe it makes sense to deliver an own python-jadepy Package to Arch AUR with vendored python packages pinned to versions known to work from https://github.com/Blockstream/Jade/blob/master/requirements.txt - this would be an inofficial solution - I could offer to do that if this is what we agree on to be the best option under this circumstances. Also it looks like I hijacked the original PR here because it probably tries to fix something different, as it did not fix the problem. |
|
Hi @1-21gigasats I think we can probably handle this particular b0rkage with a smaller commit based on yours, I'll try to get that merged for the next release (which should be pretty soon). This PR itself is dead so if you want to open a new PR feel free and I'll close this one. |
Summary
JadeSerialImpl.read()Testing
python -m unittest discover -s tests -p test_jade_serial.py -v