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

https: support 0 maxCachedSessions #4252

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Dec 12, 2015

Not sure if the could be any other description.

cc @nodejs/crypto

@indutny indutny added the https Issues or PRs related to the https subsystem. label Dec 12, 2015
@indutny
Copy link
Member Author

indutny commented Dec 12, 2015

@indutny
Copy link
Member Author

indutny commented Dec 12, 2015

CI is green, only unrelated failures.

});

process.on('exit', function() {
assert.equal(serverRequests, TOTAL_REQS);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a check for clientSessions.length here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

@bnoordhuis
Copy link
Member

LGTM. Apropos the commit log: support zero maxCachedSessions or support disabling session caching?

@jasnell
Copy link
Member

jasnell commented Dec 12, 2015

LGTM. +1 to @bnoordhuis' comment regarding the commit log.

@jasnell
Copy link
Member

jasnell commented Dec 12, 2015

@nodejs/http ... I'm leaning towards semver-minor on this. thoughts?

indutny added a commit that referenced this pull request Dec 12, 2015
Zero value of `maxCachedSessions` should disable TLS session caching in
`https.Agent`

PR-URL: #4252
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@indutny
Copy link
Member Author

indutny commented Dec 12, 2015

Landed in acef181, thank you!. @jasnell let's do minor just for safety.

@indutny indutny closed this Dec 12, 2015
@indutny indutny added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 12, 2015
@indutny indutny deleted the feature/zero-session-cache branch December 12, 2015 17:49
indutny added a commit that referenced this pull request Dec 15, 2015
Zero value of `maxCachedSessions` should disable TLS session caching in
`https.Agent`

PR-URL: #4252
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
cjihrig added a commit that referenced this pull request Dec 15, 2015
Notable changes:

* domains:
  - Fix handling of uncaught exceptions.
    (Julien Gilli) #3654.
* https:
  - Added support for disabling session caching.
    (Fedor Indutny) #4252.
* repl:
  - Allow third party modules to be imported using
    require(). This corrects a regression from 5.2.0.
    (Ben Noordhuis) #4215.
* deps:
  - Upgrade libuv to 1.8.0.
    (Saúl Ibarra Corretgé) #4276.

PR-URL: #4281
cjihrig added a commit that referenced this pull request Dec 16, 2015
Notable changes:

* buffer:
  - Buffer.prototype.includes() has been added to keep parity
    with TypedArrays. (Alexander Martin) #3567.
* domains:
  - Fix handling of uncaught exceptions.
    (Julien Gilli) #3654.
* https:
  - Added support for disabling session caching.
    (Fedor Indutny) #4252.
* repl:
  - Allow third party modules to be imported using
    require(). This corrects a regression from 5.2.0.
    (Ben Noordhuis) #4215.
* deps:
  - Upgrade libuv to 1.8.0.
    (Saúl Ibarra Corretgé) #4276.

PR-URL: #4281
cjihrig added a commit that referenced this pull request Dec 16, 2015
Notable changes:

* buffer:
  - Buffer.prototype.includes() has been added to keep parity
    with TypedArrays. (Alexander Martin) #3567.
* domains:
  - Fix handling of uncaught exceptions.
    (Julien Gilli) #3654.
* https:
  - Added support for disabling session caching.
    (Fedor Indutny) #4252.
* repl:
  - Allow third party modules to be imported using
    require(). This corrects a regression from 5.2.0.
    (Ben Noordhuis) #4215.
* deps:
  - Upgrade libuv to 1.8.0.
    (Saúl Ibarra Corretgé) #4276.

PR-URL: #4281

Conflicts:
	src/node_version.h
@shouze
Copy link

shouze commented Dec 21, 2015

@indutny would you explain me why we could/should disable session reuse? I don't see any production app use cases.

@indutny
Copy link
Member Author

indutny commented Dec 21, 2015

@shouze there is one use case for the moment, and it is related to the #3940 . Basically session cache has an API problem, that is not fixed yet. So it is very practical to disable it when people want to see server certificates.

I'm going to fix #3940, but nevertheless disabling session cache should be useful for testing and, probably, other use cases.

Cheers!

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Zero value of `maxCachedSessions` should disable TLS session caching in
`https.Agent`

PR-URL: nodejs#4252
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Notable changes:

* buffer:
  - Buffer.prototype.includes() has been added to keep parity
    with TypedArrays. (Alexander Martin) nodejs#3567.
* domains:
  - Fix handling of uncaught exceptions.
    (Julien Gilli) nodejs#3654.
* https:
  - Added support for disabling session caching.
    (Fedor Indutny) nodejs#4252.
* repl:
  - Allow third party modules to be imported using
    require(). This corrects a regression from 5.2.0.
    (Ben Noordhuis) nodejs#4215.
* deps:
  - Upgrade libuv to 1.8.0.
    (Saúl Ibarra Corretgé) nodejs#4276.

PR-URL: nodejs#4281

Conflicts:
	src/node_version.h
nicolas-moteau added a commit to Orange-OpenSource/node that referenced this pull request Mar 6, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 6, 2019
PR-URL: nodejs#26433
Refs: nodejs#2228
Refs: nodejs#4252
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
PR-URL: nodejs#26433
Refs: nodejs#2228
Refs: nodejs#4252
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
PR-URL: #26433
Refs: #2228
Refs: #4252
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants