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

ensure preload modules are always preloaded #2253

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Jul 27, 2015

For consistency -r/--require should always preload modules; right now it works with normal startup, cluster, and eval; having preloading work with stdin and interactive mode would make this more consistent.

@thefourtheye thefourtheye added the module Issues and PRs related to the module subsystem. label Jul 27, 2015
@thefourtheye
Copy link
Contributor

Will it have impact on startup time?

@Fishrock123
Copy link
Contributor

Will it have impact on startup time?

No not really.

@bmeck
Copy link
Member Author

bmeck commented Jul 27, 2015

@thefourtheye only when the flag is enabled, otherwise we are looking at a few integer checks for preloaded_module_count

@Fishrock123
Copy link
Contributor

@Fishrock123
Copy link
Contributor

Test is failing on smartos-14 (32 and 64bit):

not ok 535 - test-preload.js
# 
# assert.js:89
# throw new assert.AssertionError({
# ^
# AssertionError: '' == 'A\nhello\n'
# at ChildProcess.<anonymous> (/home/iojs/build/workspace/iojs+pr+other/nodes/smartos14-64/test/parallel/test-preload.js:78:10)
# at emitTwo (events.js:87:13)
# at ChildProcess.emit (events.js:172:7)
# at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)

@bmeck
Copy link
Member Author

bmeck commented Jul 27, 2015

@Fishrock123 even without this patch it should never be empty... that is... concerning

@Fishrock123
Copy link
Contributor

cc @nodejs/platform-solaris

@bmeck
Copy link
Member Author

bmeck commented Aug 19, 2015

is there a person I can talk to about this, as I don't run SmartOS normally, and am completely lost as to how it could be empty/worked prior to this.

@Fishrock123
Copy link
Contributor

cc @jbergstroem / @misterdjules again

@Fishrock123
Copy link
Contributor

@Fishrock123
Copy link
Contributor

Seems fine to me. @bmeck how do you feel about just ignoring it on smartos?

@thefourtheye
Copy link
Contributor

The commit message is too long and commit log has to be improved.

@bmeck
Copy link
Member Author

bmeck commented Aug 22, 2015

@thefourtheye improved how

const fixtureA = fixture('printA.js');
const fixtureB = fixture('printB.js');
const fixtureC = fixture('printC.js');
const fixtureThrows = fixture('throws_error4.js');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd prefer if you only const unmodified variables you add, but if you want to do these also I really don't mind.

@Fishrock123
Copy link
Contributor

LGTM otherwise, I'll just add some detail to the commit description if you don't. :)

@jbergstroem
Copy link
Member

I can reproduce this fail but haven't had time to look into it. Would appreciate if @misterdjules had a moment.

@bmeck
Copy link
Member Author

bmeck commented Aug 24, 2015

rebased. uses camelcase now. repro'd fail on smartos, trying to reduce it

@bmeck
Copy link
Member Author

bmeck commented Aug 31, 2015

@nodejs/platform-solaris i am unsure how to test this exactly, but is closing stdin before it is fully drained dropping the content on smartos?

@rvagg
Copy link
Member

rvagg commented Sep 10, 2015

ping @No9, perhaps you can lend an eye to this one since you're an Illumos user?

@jbergstroem
Copy link
Member

If anyone else is using illumos/smartos and would like to help out reviewing that'd be great!

@rvagg
Copy link
Member

rvagg commented Sep 11, 2015

also would love to additional people to @nodejs/platform-solaris if there are any

@No9
Copy link
Member

No9 commented Sep 11, 2015

@rvagg I'll look in over the weekend

@jbergstroem
Copy link
Member

@No9 would you be interested in being part of the small team of people that (in my case, at least tries to) debugs sunos stuff?

@No9
Copy link
Member

No9 commented Sep 11, 2015

@jbergstroem Thanks for asking
Yes I would :)

@rvagg
Copy link
Member

rvagg commented Sep 11, 2015

added to @nodejs/platform-solaris, thanks @No9!

@mhdawson
Copy link
Member

Failed on AIX in last night's run as well #6716

@bmeck
Copy link
Member Author

bmeck commented May 12, 2016

This is starting to look like some sort of race condition

@mhdawson
Copy link
Member

@bmeck @Fishrock123 on AIX it looks like a consistent failure. If it helps I can give one or both of you access to run on that machine.

@Fishrock123
Copy link
Contributor

We should also skip on AIX for now then, but we do need to investigate more.

@Fishrock123 Fishrock123 mentioned this pull request May 12, 2016
2 tasks
evanlucas pushed a commit that referenced this pull request May 17, 2016
This test fails on Solaris, see the PR for discussion.
PR-URL: #2253
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins
Copy link
Contributor

@bmeck looks like there are some regressions here. Adding dont-land for now, but please feel free to open a PR against v4.x-staging if this should be packported

@Fishrock123
Copy link
Contributor

@thealphanerd No, those are just the result of other issues.

this should be fine if it lands with #6728

@MylesBorins
Copy link
Contributor

thanks @Fishrock123 will work on that later today

@MylesBorins
Copy link
Contributor

@bmeck would you be willing to do a manual backport of this and #6728 ?

@bmeck
Copy link
Member Author

bmeck commented Jun 29, 2016

@thealphanerd onto v4?

@MylesBorins
Copy link
Contributor

indeed. It is not landing cleanly

@MylesBorins
Copy link
Contributor

ping @bmeck

@bmeck
Copy link
Member Author

bmeck commented Aug 30, 2016

@thealphanerd backported against v4.5.0 (no staging exists right now?) on https://github.com/bmeck/node/tree/backport-2253 , still flaky it seems after running tests

@MylesBorins
Copy link
Contributor

the staging is v4.x-staging

If it is still flaky then perhaps we should just mark this don't land?

@bmeck
Copy link
Member Author

bmeck commented Aug 30, 2016

@thealphanerd these are just adding tests for expected behavior, if we don't want to land it thats fine

@MylesBorins
Copy link
Contributor

@bmeck if that is the case then we should land it. Would you be willing to open a PR?

bmeck added a commit to bmeck/node that referenced this pull request Aug 30, 2016
@bmeck
Copy link
Member Author

bmeck commented Aug 30, 2016

@thealphanerd #8340

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants