Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix segfault import when multidict bundled with pyinstaller. #433

Closed
wants to merge 3 commits into from

Conversation

iemelyanov
Copy link
Contributor

@iemelyanov iemelyanov commented Dec 13, 2019

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.

@codecov
Copy link

codecov bot commented Dec 13, 2019

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
Member

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

@iemelyanov
Copy link
Contributor Author

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
Member

asvetlov commented Dec 13, 2019

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
Contributor Author

iemelyanov commented Dec 13, 2019

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
Contributor Author

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
Member

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
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
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants