Skip to content

Fix segfault import when multidict bundled with pyinstaller.#433

Closed
iemelyanov wants to merge 3 commits intomasterfrom
fix-segfault-when-bundled-with-pyinstaller
Closed

Fix segfault import when multidict bundled with pyinstaller.#433
iemelyanov wants to merge 3 commits intomasterfrom
fix-segfault-when-bundled-with-pyinstaller

Conversation

@iemelyanov
Copy link
Copy Markdown
Contributor

@iemelyanov iemelyanov commented Dec 13, 2019 •
edited
Loading

What do these changes do?

Fix segfault import when multidict bundled with pyinstaller.

Are there changes in behavior for the user?

No

Related issue number

#432

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

@iemelyanov iemelyanov requested a review from asvetlov as a code owner December 13, 2019 10:34
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 13, 2019 •
edited
Loading

Codecov Report

Merging #433 into master will increase coverage by 3.32%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   75.81%   79.14%   +3.32%     
==========================================
  Files           5        5              
  Lines         488      489       +1     
==========================================
+ Hits          370      387      +17     
+ Misses        118      102      -16
Impacted Files Coverage Δ
multidict/__init__.py 90% <100%> (+1.11%) ⬆️
multidict/_multidict_base.py 13.91% <0%> (+13.91%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 050f3c4...e1c4a7a. Read the comment docs.

@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Dec 13, 2019
@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Dec 13, 2019

Sorry, I don't understand how it fixes the project.
Why did it fail before?

@iemelyanov
Copy link
Copy Markdown
Contributor Author

iemelyanov commented Dec 13, 2019

Sorry, I don't understand how it fixes the project.
Why did it fail before?

Mb better split this fix on two.

  1. Fix segfault which happened when failed get attribute https://github.com/aio-libs/multidict/blob/master/multidict/_multidict.c#L1494 by deleting decrefs of static objects.
  2. Fix incorrect get attribute.

One interesting thing is that multidict correct imported and work event if internally happened fail and a big part of code in PyInit__multidict not executed!

@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Dec 13, 2019 •
edited
Loading

Things that bother me:

  1. _mdrepr is imported from multidict._multidict_base on the current master, why additional import in __init__.py is needed?
  2. If the import of multidict._multidict module fails PyInit__multidict returns NULL, the module object is even not created. Why pyinstaller thinks that the import was succeded. I expect some kind of ImportError exception raising.

In conclusion, pyinstaller's import machinery looks buggy at least partially.
I would know what part is broken. For now, the fix looks like making really very weird things to slip between pyinstaller bugs.
Please correct me if I'm wrong.

@iemelyanov
Copy link
Copy Markdown
Contributor Author

iemelyanov commented Dec 13, 2019 •
edited by asvetlov
Loading

Things that bother me:

  1. _mdrepr is imported from multidict._multidict_base on the current master, why additional import in __init__.py is needed?
  2. If the import of multidict._multidict module fails PyInit__multidict returns NULL, the module object is even not created. Why pyinstaller thinks that the import was succeded. I expect some kind of ImportError exception raising.

In conclusion, pyinstaller's import machinery looks buggy at least partially.
I would know what part is broken. For now, the fix looks like making really very weird things to slip between pyinstaller bugs.
Please correct me if I'm wrong.

You right. For me, this fails looks strange too and mb we should more deeply investigate this issue.

@iemelyanov
Copy link
Copy Markdown
Contributor Author

iemelyanov commented Dec 13, 2019

I propose to close this PR and live #433 open until we don't understand what's going on in pyinstaller and why import multidict work event if fail happen here - https://github.com/aio-libs/multidict/blob/master/multidict/_multidict.c#L1494. What do you think?

@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Dec 13, 2019

Agree

@iemelyanov iemelyanov closed this Dec 13, 2019
@asvetlov asvetlov deleted the fix-segfault-when-bundled-with-pyinstaller branch December 19, 2019 20:45
aio-libs-github-bot Bot pushed a commit that referenced this pull request Jan 18, 2021
@dependabot
Bump pytest-cov from 2.10.1 to 2.11.0 (#562)
a6a8527 
Bumps [pytest-cov](https://github.com/pytest-dev/pytest-cov) from 2.10.1 to 2.11.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst">pytest-cov's changelog</a>.</em></p>
<blockquote>
<h2>2.11.0 (2021-01-18)</h2>
<ul>
<li>Bumped minimum coverage requirement to 5.2.1. This prevents reporting issues.
Contributed by Mateus Berardo de Souza Terra in <code>[#433](pytest-dev/pytest-cov#433) &lt;https://github.com/pytest-dev/pytest-cov/pull/433&gt;</code>_.</li>
<li>Improved sample projects (from the <code>examples &lt;https://github.com/pytest-dev/pytest-cov/tree/master/examples&gt;</code>_
directory) to support running <code>tox -e pyXY</code>. Now the example configures a suffixed coverage data file,
and that makes the cleanup environment unnecessary.
Contributed by Ganden Schaffner in <code>[#435](pytest-dev/pytest-cov#435) &lt;https://github.com/pytest-dev/pytest-cov/pull/435&gt;</code>_.</li>
<li>Removed the empty <code>console_scripts</code> entrypoint that confused some Gentoo build script.
I didn't ask why it was so broken cause I didn't want to ruin my day.
Contributed by Michał Górny in <code>[#434](pytest-dev/pytest-cov#434) &lt;https://github.com/pytest-dev/pytest-cov/pull/434&gt;</code>_.</li>
<li>Fixed the missing <code>coverage context &lt;https://coverage.readthedocs.io/en/stable/contexts.html&gt;</code>_
when using subprocesses.
Contributed by Bernát Gábor in <code>[#443](pytest-dev/pytest-cov#443) &lt;https://github.com/pytest-dev/pytest-cov/pull/443&gt;</code>_.</li>
<li>Updated the config section in the docs.
Contributed by Pamela McA'Nulty in <code>[#429](pytest-dev/pytest-cov#429) &lt;https://github.com/pytest-dev/pytest-cov/pull/429&gt;</code>_.</li>
<li>Migrated CI to travis-ci.com (from .org).</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/b45388d0bf81465759412c6b977c6a1fc5201346"><code>b45388d</code></a> Bump version: 2.10.1 → 2.11.0</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/bd7e8506d7c0d85ac10f3569b8da3584b80bc67c"><code>bd7e850</code></a> Fix link name.</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/5f935d5dc281b470687147b28f90b09dc887efc3"><code>5f935d5</code></a> Update changelog.</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/d3c382a7e7e933e9602f5a5445d03f6dc5138da0"><code>d3c382a</code></a> Skip this on 3.8+</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/a2493d5f1825568b1f66ff033a49f28437560cad"><code>a2493d5</code></a> Skip this on 3.8+</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/25eed212085ce9a2d5383a6a4a2b360d0d514f89"><code>25eed21</code></a> Turns out there were some internal changes in the pytester plugin.</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/42d7705b26c3fdbd74cd81a01b8fe0f213d16095"><code>42d7705</code></a> Oops.</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/4ce7ac3cb463e7a42b1f10da45aba3c5987d23de"><code>4ce7ac3</code></a> Update test deps.</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/0eada98182c7e4754927d5eefdd5e1ae72a9062d"><code>0eada98</code></a> Update skel; migrate to travis-ci.com.</li>
<li><a href="https://github.com/pytest-dev/pytest-cov/commit/6592810f4181091fbd284e013b5ed91c08e3a479"><code>6592810</code></a> Update setup.py</li>
<li>Additional commits viewable in <a href="https://github.com/pytest-dev/pytest-cov/compare/v2.10.1...v2.11.0">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pytest-cov&package-manager=pip&previous-version=2.10.1&new-version=2.11.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

No one assigned

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iemelyanov @asvetlov