This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12627

worker: adapt _blockingreader to work around a python3.8.[0-1] bug (issue6444)
ClosedPublic

Authored by mharbison72 on Tue, May 17, 3:33 PM.

Details

Reviewers
mjacob
Alphare
Group Reviewers
hg-reviewers
Commits
rHG2fe4efaa59af: worker: adapt _blockingreader to work around a python3.8.[0-1] bug (issue6444)
Summary

Python 3.8.0 is the latest I can load on Ubuntu 18.04, and I regularly hit the
TypeError because this function is missing. While it can be avoided by
disabling worker usage via config option, that's a bit obscure.

I'm limiting the function definition to the narrow range of affected pythons
because there were other bugs in this area that were worked around, that I don't
fully understand. See the bug report for discussions on why the narrow range,
and related commits working around other bugs.

Diff Detail

Repository
rHG Mercurial
Branch
stable
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

mharbison72 created this revision.Tue, May 17, 3:33 PM
mharbison72 added a comment.Tue, May 17, 3:34 PM

This is meant for stable.

The conflict resolution when merging this into default is a simple de-indent.

mjacob requested changes to this revision.Tue, May 17, 11:31 PM
mjacob added a subscriber: mjacob.
mjacob added inline comments.
mercurial/worker.py
86

This could change the size of b, which shouldn’t be done.

If pickle calls readinto() (e.g. on my machine, this happens when the pickled data contains a bytestring >= 2**16 bytes long), it passes a memoryview which doesn’t allow changing its size. Because the size of b is the size of the pickled object and readall() returns all bytes in the stream, containing the bytes for the pickled object + at least one trailing byte, this line would always raise a “ValueError: memoryview assignment: lvalue and rvalue have different structures” exception.

What I suggest instead is to mostly copy-paste the implementation of read() and modify it such that it reads into the provided buffer instead of creating one.

This revision now requires changes to proceed.Tue, May 17, 11:31 PM
martinvonz added subscribers: Alphare, martinvonz.Wed, May 18, 11:32 AM

I *think* you're supposed to open a "merge request" on https://foss.heptapod.net/mercurial/mercurial-devel these days. Right, @Alphare?

mharbison72 added a comment.Wed, May 18, 3:59 PM
In D12627#193334, @martinvonz wrote:

I *think* you're supposed to open a "merge request" on https://foss.heptapod.net/mercurial/mercurial-devel these days. Right, @Alphare?

I didn't think we made the switch yet, but I'm fine doing that as a pilot. (I ended up amending a commit comment typo since the last push, so it will need to run CI again.)

martinvonz added a comment.Wed, May 18, 4:01 PM
In D12627#193347, @mharbison72 wrote:
In D12627#193334, @martinvonz wrote:

I *think* you're supposed to open a "merge request" on https://foss.heptapod.net/mercurial/mercurial-devel these days. Right, @Alphare?

I didn't think we made the switch yet, but I'm fine doing that as a pilot. (I ended up amending a commit comment typo since the last push, so it will need to run CI again.)

The reason I suggested it was that I queued the patch and tried to push it to hg-committed, but that failed and told me to create a merge request instead. So I suppose it's actually you who should create that merge request since it's your patch. I'm not sure I understood it correctly, though.

Alphare requested changes to this revision.Wed, May 18, 7:16 PM

I *think* you're supposed to open a "merge request" on https://foss.heptapod.net/mercurial/mercurial-devel these days. Right, @Alphare?

Right!

I didn't think we made the switch yet, but I'm fine doing that as a pilot. (I ended up amending a commit comment typo since the last push, so it will need to run CI again.)

I have actually already merged a couple of MRs using Heptapod.

The reason I suggested it was that I queued the patch and tried to push it to hg-committed, but that failed and told me to create a merge request instead. So I suppose it's actually you who should create that merge request since it's your patch. I'm not sure I understood it correctly, though.

Yup. Phabricator is deprecated and slated for read-only state on/around June 1st. The deprecation means that any new patches should be sent to Heptapod unless absolutely infeasible (because something is terribly broken), and old patches will be reviewed here, then put in a topic by yours truly for a CI pass and merged accordingly. Any diff that still needs review/turn-around by June 1st will need to be resent through Heptapod.

@mharbison72 you should have no trouble at all since you're already using Hetpapod for the CI. Please read the contributor guide here: https://www.mercurial-scm.org/wiki/Heptapod

I hope I'm clear, it's very late and my brain is half-working. :)

This revision now requires changes to proceed.Wed, May 18, 7:16 PM
mharbison72 added a comment.Thu, May 19, 11:17 AM
In D12627#193350, @Alphare wrote:

@mharbison72 you should have no trouble at all since you're already using Hetpapod for the CI. Please read the contributor guide here: https://www.mercurial-scm.org/wiki/Heptapod
I hope I'm clear, it's very late and my brain is half-working. :)

Done. I couldn't remember where I saw these instructions, and I created it before you responded here, but I think I did it right. I don't see a "Ready" button now, but I do see one "Mark as Draft", so I assume it's already in that state.

Is there any way to link that wiki page from the top of the page that creates the merge request, for easier discoverability by new users?

baymax updated this revision to Diff 33418.Fri, May 20, 11:14 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

baymax updated this revision to Diff 33447.Tue, May 24, 12:43 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

mharbison72 added a commit: rHG2fe4efaa59af: worker: adapt _blockingreader to work around a python3.8.[0-1] bug (issue6444).Tue, May 24, 12:57 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG2fe4efaa59af: worker: adapt _blockingreader to work around a python3.8.[0-1] bug (issue6444) (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/worker.py (17 lines)

Diff 33407

mercurial/worker.py

class _blockingreader(object): class _blockingreader(object):
def __init__(self, wrapped): def __init__(self, wrapped):
self._wrapped = wrapped self._wrapped = wrapped
# Do NOT implement readinto() by making it delegate to # Do NOT implement readinto() by making it delegate to
# _wrapped.readinto(), since that is unbuffered. The unpickler is fine # _wrapped.readinto(), since that is unbuffered. The unpickler is fine
# with just read() and readline(), so we don't need to implement it. # with just read() and readline(), so we don't need to implement it.
if (3, 8, 0) <= sys.version_info[:3] < (3, 8, 2):
# This is required for python 3.8, prior to 3.8.2. See issue6444.
def readinto(self, b):
pos = 0
size = len(b)
mjacobUnsubmitted
Not Done

This could change the size of b, which shouldn’t be done.

If pickle calls readinto() (e.g. on my machine, this happens when the pickled data contains a bytestring >= 2**16 bytes long), it passes a memoryview which doesn’t allow changing its size. Because the size of b is the size of the pickled object and readall() returns all bytes in the stream, containing the bytes for the pickled object + at least one trailing byte, this line would always raise a “ValueError: memoryview assignment: lvalue and rvalue have different structures” exception.

What I suggest instead is to mostly copy-paste the implementation of read() and modify it such that it reads into the provided buffer instead of creating one.

mjacob: This could change the size of `b`, which shouldn’t be done. If pickle calls `readinto()` (e.g.
while pos < size:
ret = self._wrapped.readinto(b[pos:])
if ret is None:
continue # nonblocking, and no data
elif ret == 0:
break # 0 -> EOF
pos += ret
return pos
def readline(self): def readline(self):
return self._wrapped.readline() return self._wrapped.readline()
# issue multiple reads until size is fulfilled # issue multiple reads until size is fulfilled
def read(self, size=-1): def read(self, size=-1):
if size < 0: if size < 0:
return self._wrapped.readall() return self._wrapped.readall()