fix: remove max Content-Length middleware #16
Conversation
| @@ -13,7 +13,8 @@ import { BlockBatch } from './block-batch.js' | |||
| * @typedef {import('../bindings').R2Bucket} R2Bucket | |||
| */ | |||
|
|
|||
| const MAX_BLOCK_LENGTH = 1024 * 1024 * 4 | |||
| // 2MB (max safe libp2p block size) + typical block header length + some leeway | |||
| const MAX_ENCODED_BLOCK_LENGTH = (1024 * 1024 * 2) + 39 + 61 | |||
There was a problem hiding this comment.
We don't know how much to read from the CAR, but in theory no single block should be bigger than this.
src/lib/block-batch.js
Outdated
| const batch = [] | ||
| let last = offsets[0] | ||
| let last = queue[0] |
There was a problem hiding this comment.
last is first item in queue? i guess it's the last from the previous batch, but it's a little confusing that we take the head of the queue and call it last.
There was a problem hiding this comment.
also why not shift this guy off the queue and add him to the batch immediately, otherwise our first comparison is always with itself!
src/lib/block-batch.js
Outdated
| const item = queue.shift() | ||
| if (!item) break | ||
| if (item.carCid.toString() !== last.carCid.toString() || item.offset - last.offset >= MAX_BYTES_BETWEEN) { | ||
| queue.unshift(item) // not in this batch, return to pile |
There was a problem hiding this comment.
minor but could skip a gozzy shift/unshift operation and invert the logic to make it more positive... like test for the condition we want to be true
| const item = queue.shift() | |
| if (!item) break | |
| if (item.carCid.toString() !== last.carCid.toString() || item.offset - last.offset >= MAX_BYTES_BETWEEN) { | |
| queue.unshift(item) // not in this batch, return to pile | |
| ```suggestion | |
| const item = queue.at(0) | |
| if (!item) break | |
| if (item.carCid.toString() === last.carCid.toString() && item.offset - last.offset < MAX_BYTES_BETWEEN) { | |
| batch.push(queue.shift(item)) // winner! remove from queue and add to item |
| const carPath = `${carCid}/${carCid}.car` | ||
| const range = { | ||
| offset: batch[0].offset, | ||
| length: batch[batch.length - 1].offset - batch[0].offset + MAX_ENCODED_BLOCK_LENGTH |
There was a problem hiding this comment.
| length: batch[batch.length - 1].offset - batch[0].offset + MAX_ENCODED_BLOCK_LENGTH | |
| length: batch.at(-1).offset - batch[0].offset + MAX_ENCODED_BLOCK_LENGTH |
src/lib/blockstore.js
Outdated
| if (!res) { | ||
| for (const blocks of pendingBlocks.values()) { | ||
| blocks.forEach(b => b.resolve()) | ||
| } | ||
| return |
There was a problem hiding this comment.
plz comment to explain this! From convo this is to furfil the contract of returning undefined if we dont have the block
Also removes the memory budget code (which has no bearing on actual memory usage!).