This repository was archived by the owner on Feb 26, 2024. It is now read-only.
Only remove listeners left around by soljson #4092
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Another approach might be to look at the actual version of solc, but I went with this because the explicit set comparison will handle the problem more generally... and cover nightlies and such, so we won't need to identify safe/unsafe versions so rigorously. |
|
Maybe both reprod
use-truffle-core
truffle init
cat << EOF > ./migrations/1_initial_migration.js
const Migrations = artifacts.require("Migrations");
module.exports = function (deployer) {
Promise.reject(4242);
deployer.deploy(Migrations);
};
EOF
truffle test |
cds-amal
reviewed
Jun 5, 2021
packages/compile-solidity/compilerSupplier/loadingStrategies/LoadingStrategy.js
Outdated
Show resolved
Hide resolved
while reviewing @gnidan's pr, I wanted to test the behavior of current soljson as scaffolded by `truffle init` with a contrived unhandledRejection. That is, adding `Promise.reject(4242)` to a migrations deploy script. depending on the version soljson registers for `uncaughtException` as well as `unhandledRejection`. However BOTH handlers will abort and dump emscripten large generated source to stdout. yuck.
This is controversial, but I think our use case, and space requires erring on the side of caution. [See this discusion](nodejs/node#22822) for more perspective.
- [x] truffle migrate - [ ] truffle test - [ ] truffle compile - [ ] truffle debug - [ ] truffle console - [ ] truffle migrate - [ ] others?
davidmurdoch
reviewed
Jun 7, 2021
packages/compile-solidity/compilerSupplier/loadingStrategies/Local.js
Outdated
Show resolved
Hide resolved
Also remove `unhandledRejection` to soljson wrapper
cds-amal
suggested changes
Jun 10, 2021
packages/compile-solidity/compilerSupplier/loadingStrategies/Local.js
Outdated
Show resolved
Hide resolved
packages/compile-solidity/compilerSupplier/loadingStrategies/VersionRange.js
Outdated
Show resolved
Hide resolved
packages/compile-solidity/compilerSupplier/loadingStrategies/VersionRange.js
Outdated
Show resolved
Hide resolved
Co-authored-by: amal <cds.sudama@gmail.com>
cds-amal
approved these changes
Jun 10, 2021
|
Nice one @gnidan ! |
Comment on lines
94
to
100
| const newListeners = listeners.filter( | ||
| (listener) => !markedListeners[eventName].has(listener), | ||
| ); | ||
|
|
||
| if (execeptionHandler) { | ||
| process.removeListener("uncaughtException", execeptionHandler); | ||
| for (const listener of newListeners) { | ||
| process.removeListener(eventName, listener); | ||
| } |
There was a problem hiding this comment.
This could be condensed to:
for (const eventName in markedListeners) {
const marked = markedListeners[eventName];
process.listeners(eventName).forEach((listener) => {
if (!marked.has(listener)) {
process.removeListener(eventName, listener);
}
});
}And if we change markedListeners to a Map<string, Array<AnyFunction> it could be:
for (const [eventName, marked] of markedListeners) {
process.listeners(eventName).forEach((listener) => {
if (!marked.has(listener)) {
process.removeListener(eventName, listener);
}
});
}There was a problem hiding this comment.
Borrowed the first one :) Thanks @davidmurdoch!
davidmurdoch
approved these changes
Jun 11, 2021
|
(I feel like there's a joke here that begins "how many engineers does it take to add a mutex lock?" :) Thanks @cds-amal and @davidmurdoch! Will merge when green. |
cds-amal
approved these changes
Jun 11, 2021
Comment on lines
+92
to
+97
| for (const eventName in markedListeners) { | ||
| const marked = markedListeners[eventName]; | ||
| for (const listener of process.listeners(eventName)) { | ||
| if (!marked.has(listener)) { | ||
| process.removeListener(eventName, listener); | ||
| } |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So, here's a hypothesis for the
uncaughtExceptionbusiness in compile-solidity.Solidity upgraded Emscripten from v1.35.4 to v1.37.21 in solc v0.5.1 (in this PR). Inspecting the Emscripten changelog, this issue was closed as part of v1.36.6. Looks like the fix here may have changed buggy behavior that left
uncaughtExceptions to linger.Since we support old Solidity and need to handle pre-v0.5.1 behavior, this PR proposes that we alter the listener removal by first capturing the listeners that existed before loading solc and then removing only those listeners that are new after loading solc.
This is a different proposed solution than #4091... I haven't done any digging re: that PR's hypothesis that we must remove listeners for
unhandledRejection, but I suspect that may be a red herring. This PR should address the "exits violently" part of #4090 (as well as many other issues over the past however-long)