-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
http2: implement maxSessionMemory #17967
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,6 +82,9 @@ void inline debug_vfprintf(const char* format, ...) { | |
// Also strictly limit the number of outstanding SETTINGS frames a user sends | ||
#define DEFAULT_MAX_SETTINGS 10 | ||
|
||
// Default maximum total memory cap for Http2Session. | ||
#define DEFAULT_MAX_SESSION_MEMORY 1e7; | ||
|
||
// These are the standard HTTP/2 defaults as specified by the RFC | ||
#define DEFAULT_SETTINGS_HEADER_TABLE_SIZE 4096 | ||
#define DEFAULT_SETTINGS_ENABLE_PUSH 1 | ||
|
@@ -501,8 +504,17 @@ class Http2Options { | |
return max_outstanding_settings_; | ||
} | ||
|
||
void SetMaxSessionMemory(uint64_t max) { | ||
max_session_memory_ = max; | ||
} | ||
|
||
uint64_t GetMaxSessionMemory() { | ||
return max_session_memory_; | ||
} | ||
|
||
private: | ||
nghttp2_option* options_; | ||
uint64_t max_session_memory_ = DEFAULT_MAX_SESSION_MEMORY; | ||
uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; | ||
padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE; | ||
size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; | ||
|
@@ -629,6 +641,9 @@ class Http2Stream : public AsyncWrap, | |
// Returns the stream identifier for this stream | ||
inline int32_t id() const { return id_; } | ||
|
||
inline void IncrementAvailableOutboundLength(size_t amount); | ||
inline void DecrementAvailableOutboundLength(size_t amount); | ||
|
||
inline bool AddHeader(nghttp2_rcbuf* name, | ||
nghttp2_rcbuf* value, | ||
uint8_t flags); | ||
|
@@ -848,7 +863,7 @@ class Http2Session : public AsyncWrap { | |
inline void AddStream(Http2Stream* stream); | ||
|
||
// Removes a stream instance from this session | ||
inline void RemoveStream(int32_t id); | ||
inline void RemoveStream(Http2Stream* stream); | ||
|
||
// Write data to the session | ||
inline ssize_t Write(const uv_buf_t* bufs, size_t nbufs); | ||
|
@@ -906,6 +921,30 @@ class Http2Session : public AsyncWrap { | |
Http2Settings* PopSettings(); | ||
bool AddSettings(Http2Settings* settings); | ||
|
||
void IncrementCurrentSessionMemory(uint64_t amount) { | ||
current_session_memory_ += amount; | ||
} | ||
|
||
void DecrementCurrentSessionMemory(uint64_t amount) { | ||
current_session_memory_ -= amount; | ||
} | ||
|
||
// Returns the current session memory including the current size of both | ||
// the inflate and deflate hpack headers, the current outbound storage | ||
// queue, and pending writes. | ||
uint64_t GetCurrentSessionMemory() { | ||
uint64_t total = current_session_memory_ + sizeof(Http2Session); | ||
total += nghttp2_session_get_hd_deflate_dynamic_table_size(session_); | ||
total += nghttp2_session_get_hd_inflate_dynamic_table_size(session_); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m wondering what the cost of these calls is, since they can’t be inlined & this function is called quite a few times as far as I can tell? Did you try to measure that? (Not a blocker, I think this feature is worth it either way) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, haven't benchmarked it yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One way we can limit the impact here is to capture the inbound dynamic table size at the completion of a headers frame, and the outbound dynamic size after sending a headers frame, then just add the static values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (note that doing so relaxes one of the protections included here that keeps headers within a single HEADERS frame from blowing the memory limit) |
||
total += outgoing_storage_.size(); | ||
return total; | ||
} | ||
|
||
// Return true if current_session_memory + amount is less than the max | ||
bool IsAvailableSessionMemory(uint64_t amount) { | ||
return GetCurrentSessionMemory() + amount <= max_session_memory_; | ||
} | ||
|
||
struct Statistics { | ||
uint64_t start_time; | ||
uint64_t end_time; | ||
|
@@ -1035,6 +1074,10 @@ class Http2Session : public AsyncWrap { | |
// The maximum number of header pairs permitted for streams on this session | ||
uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; | ||
|
||
// The maximum amount of memory allocated for this session | ||
uint64_t max_session_memory_ = DEFAULT_MAX_SESSION_MEMORY; | ||
uint64_t current_session_memory_ = 0; | ||
|
||
// The collection of active Http2Streams associated with this session | ||
std::unordered_map<int32_t, Http2Stream*> streams_; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
if (!common.hasCrypto) | ||
common.skip('missing crypto'); | ||
|
||
const http2 = require('http2'); | ||
|
||
// Test that maxSessionMemory Caps work | ||
|
||
const largeBuffer = Buffer.alloc(1e6); | ||
|
||
const server = http2.createServer({ maxSessionMemory: 1 }); | ||
|
||
server.on('stream', common.mustCall((stream) => { | ||
stream.respond(); | ||
stream.end(largeBuffer); | ||
})); | ||
|
||
server.listen(0, common.mustCall(() => { | ||
const client = http2.connect(`http://localhost:${server.address().port}`); | ||
|
||
{ | ||
const req = client.request(); | ||
|
||
req.on('response', () => { | ||
// This one should be rejected because the server is over budget | ||
// on the current memory allocation | ||
const req = client.request(); | ||
req.on('error', common.expectsError({ | ||
code: 'ERR_HTTP2_STREAM_ERROR', | ||
type: Error, | ||
message: 'Stream closed with error code 11' | ||
})); | ||
req.on('close', common.mustCall(() => { | ||
server.close(); | ||
client.destroy(); | ||
})); | ||
}); | ||
|
||
req.resume(); | ||
req.on('close', common.mustCall()); | ||
} | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case you really should be specifying that you
1 MB
is one million bytes, not 2^10 bytes ;)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax ... sorry, I'm being a bit thick... can you clarify the specific change? comment or code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasnell Sorry, missed the notification – I meant, for some people 1 MB means 1.000.000 bytes, and for some it means 1.048.576 bytes, so it would be good to have disambiguated this in the documentation :)