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

[wip] merge v0.10 #267

Closed
wants to merge 2 commits into from
Closed

[wip] merge v0.10 #267

wants to merge 2 commits into from

Conversation

piscisaureus
Copy link
Contributor

Many patches have been ported over from the node/v0.10 branch onto io.js. Others are in this pull request. The following still need to be looked at.

Once all these commits have either been ported or rejected, io.js is up-to-date up to nodejs/node-v0.x-archive@fe20196 and we can construct a new merge base.

Child process fixes: (@sam-github @cjihrig)

TLS related (@indutny @bnoordhuis)

Uncaught exceptions swallowed

Timers (see also #268)

Probably not needed

Misc (@piscisaureus)

@@ -216,6 +219,11 @@ email.md: ChangeLog tools/email-footer.md
blog.html: email.md
cat $< | ./$(NODE_EXE) tools/doc/node_modules/.bin/marked > $@

doc-branch-upload: NODE_DOC_VERSION = v$(shell $(PYTHON) tools/getnodeversion.py | cut -f1,2 -d.)
Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis removed upload targets in 73b9323, which I'm +1 on, I'll either PR to add some new ones back or we'll keep them separate. We should keep that consistent and remove this target.

@piscisaureus piscisaureus force-pushed the mergev010 branch 7 times, most recently from 8848675 to 44ddf76 Compare January 10, 2015 21:40
@bnoordhuis
Copy link
Member

Regarding the SSL/TLS commits, only the first two and last three seem worth taking. The others are all about making SSLv[23] support configurable at run-time and I don't think that's a path we want to take.

Personally, I would suggest ripping out SSLv2 support entirely and putting SSLv3 behind a configure flag that prints a big warning.

/cc @indutny Agree/disagree?

@rvagg
Copy link
Member

rvagg commented Jan 11, 2015

+1, what was the argument for keeping SSLv2? IE6?

@bnoordhuis
Copy link
Member

@rvagg I don't know, it's probably pure inertia. SSLv2 was considered broken almost immediately after its introduction in 1995, that's why SSLv3 was created shortly after. There really seems no reason to keep the support around.

@bnoordhuis
Copy link
Member

@piscisaureus The uncaught exception changes seem pointless, they work around an issue when --abort_on_uncaught_exception is passed on the command line, effectively nullifying the flag.

When you tell a GP 'it hurts when I do this', they'll tell you to stop doing that. Tell a programmer, they'll add a hack.

@bnoordhuis
Copy link
Member

@piscisaureus The timer commits can be omitted when some of the suggestions from #268 get implemented (because that practically constitutes a full rewrite, nullifying the issues those commits address.)

@piscisaureus
Copy link
Contributor Author

@bnoordhuis Thanks for your replies. I agree with most of them except

putting SSLv3 behind a configure flag that prints a big warning.

It's not that I think using SSLv3 is a good idea, but I'm generally opposed to the idea of putting features behind a configure-time flag. It assumes that people build iojs themselves, which generally speaking only a very small fraction of the user base does; most people will get it from a package manager or installer. So we should choose: we either remove support for SSLv3 entirely, or we make a run-time decision possible.

Let me add that although you've indeed removed SSLv2 support a year ago, this change never saw the light of day in the form of a stable release.

@bnoordhuis
Copy link
Member

I'm in the 'secure by default' camp, +1 for dropping SSLv3 support. @iojs/tc, please chime in.

@rvagg
Copy link
Member

rvagg commented Jan 11, 2015

+1 for dropping SSLv3 from me

@chrisdickinson
Copy link
Contributor

@bnoordhuis re: the uncaught exception handler: there was a specific issue that this was handling -- specifically, I think it was that using domains at the same time as --abort-on-uncaught-exception would keep the process from aborting and producing a core. @trevnorris might remember the details than I do at the moment. Here's the PR that introduced those changes.

@chrisdickinson
Copy link
Contributor

Re: disabling SSLv3 entirely: I think this is a good opportunity to turn it off by default. If a lot of folks complain, we can go the runtime configuration route in a follow-up minor release. I'm -1 on a configure flag, for the same reason as @piscisaureus.

@rvagg rvagg added this to the v1.0.0 milestone Jan 12, 2015
@rvagg rvagg mentioned this pull request Jan 12, 2015
14 tasks
@bnoordhuis
Copy link
Member

@chrisdickinson Re: --abort-on-uncaught-exception, I think a much easier - and better - solution is to tell users to not use that flag. I can put together a PR that makes iojs print a warning on start-up or require('domain'):

WARNING: --abort-on-uncaught-exception is incompatible with the domains module

@indutny
Copy link
Member

indutny commented Jan 12, 2015

@piscisaureus
Copy link
Contributor Author

^ @bnoordhuis

@bnoordhuis
Copy link
Member

@indutny @piscisaureus I mentioned up-thread that the first two and last three may be worth cherry-picking. nodejs/node-v0.x-archive@b9283cf is not because we won't be merging the SSL2_ENABLE/SSL3_ENABLE patches, right?

@indutny
Copy link
Member

indutny commented Jan 12, 2015

Ok, good!

Julien Gilli and others added 2 commits January 13, 2015 03:29
Add a test that goes through the whole matrix of:
- command line options (--enable-ssl*)
- secureOptions
- secureProtocols

and makes sure that compatible test setups actually work as expected.

The test works by spawning two processes for each test case: one client
and one server. The test passes if a SSL/TLS connection from the client
to the server is successful and the test case was supposed to pass, or
if the connection couldn't be established and the test case was supposed
to fail.

The test is currently located in the directory 'test/external' because
it has external dependencies.

Cherry-picked-from: nodejs/node-v0.x-archive@8d045a3
@piscisaureus
Copy link
Contributor Author

@rvagg something in fasion of 64086a9 may also be useful for us.

@rvagg
Copy link
Member

rvagg commented Jan 13, 2015

@piscisaureus tbh I don't really see the utility, it's providing a version 0.10 to the docs rather than the full process.version, unless I'm missing something. I'd rather it be explicit.

@rvagg
Copy link
Member

rvagg commented Jan 13, 2015

actually this is an issue for the new docs team to take up! I delegate the decision to them, whoever they be / @chrisdickinson

@chrisdickinson
Copy link
Contributor

@bnoordhuis Re: abort-on-uncaught-exception, I think that approach might preclude anyone using hapi from making use of post-mortem debugging. Though, this may not be a problem if there are plans to rip domains out of hapi -- cc'ing @geek to see if that's the plan or not.

@rvagg I'm not entirely sold on including that commit, either. I'm currently leaning towards giving the doctool responsibility for the entirety of the out/doc folder, which implies that it could be run separately from iojs' make doc, and probably given its own version explicitly.

@Fishrock123
Copy link
Contributor

Is this still in progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants