Patchwork D8928: worker: don't expose readinto() on _blockingreader since pickle is picky

mail settings
Submitter phabricator
Date Aug. 15, 2020, 5:40 a.m.
Message ID <>
Download mbox | patch
Permalink /patch/47028/
State Superseded
Headers show


phabricator - Aug. 15, 2020, 5:40 a.m.
martinvonz created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

  The `pickle` module expects the input to be buffered and a whole
  object to be available when `pickle.load()` is called, which is not
  necessarily true when we send data from workers back to the parent
  process (i.e., it seems like a bad assumption for the `pickle` module
  to make). We added a workaround for that in, which made `read()` continue
  until all the requested bytes have been read.
  As we found out at work after a lot of investigation (I've spent the
  last two days on this), the native version of `pickle.load()` has
  started calling `readinto()` on the input since Python 3.8. That
  started being called in
  (and only by the C version of `pickle.load()`)). Before that, it was
  only `read()` and `readline()` that were called. The problem with that
  was that `readinto()` on our `_blockingreader` was simply delegating
  to the underlying, *unbuffered* object. The symptom we saw was that
  `hg fix` started failing sometimes on Python 3.8 on Mac. I failed very
  relyable in some cases. I still haven't figured out under what
  circumstances it fail and I've been unable to reproduce it in test
  cases (I've tried writing larger amounts of data, using different
  numbers of workers, and making the formatters sleep). I have, however,
  been able to reproduce it a 3-4 times on Linux, but then it stopped
  reproducing on the following few hundred attempts.
  To fix the problem, we can simply remove the implementation of
  `readinto()`, since the unpickler will then fall back to calling
  `read()`. (The fallback was added a bit later, in

  rHG Mercurial





To: martinvonz, #hg-reviewers
Cc: mercurial-patches, mercurial-devel


diff --git a/mercurial/ b/mercurial/
--- a/mercurial/
+++ b/mercurial/
@@ -71,8 +71,8 @@ 
         def __init__(self, wrapped):
             self._wrapped = wrapped
-        def __getattr__(self, attr):
-            return getattr(self._wrapped, attr)
+        def readline(self):
+            return self._wrapped.readline()
         # issue multiple reads until size is fulfilled
         def read(self, size=-1):