This presupposes that neither the key nor the val contain null characters. That may not be a safe assumption to make

@jasnell #14723 (comment)

Yeah, saw that as I read further :-)

A big part of the reason this was an array is because there will be a third value that needs to be passed in at some point... specifically, a flag that indicates whether or not the header pair should be stored in the header compression table

I suppose we can add another NUL-delimited column if we were to implement that.

We could even just add a single character/byte per header to the string.

single byte (bit-flag) per header should work.

In fact, why don't we go ahead and add that extra byte now as a reserved field in the structure.

One idea for the API... right now headers are set as an object.. e.g.

{
  ':status': 200,
  'content-type': 'text/plain', 
  'foo': 'bar'
}

We could (not saying should) allow the value to be wrapped somehow... e.g.

{
  ':status': 200,
  'content-type': http2.flaggedHeader('text/plain', { store: false }), 
  'foo': 'bar'
}

This feels a bit awkward but it optimizes for the common case and API familiarity and only adds the weirdness when someone really does want to use the more advanced feature.

Or, alternatively... (just brainstorming here ;-) ...)

{
  ':status': 200,
  'foo': 'bar',
  [http2.noStore]: {
    'content-type': 'text/plain'
  }
}

Where 'http2.noStore is a Symbol. This still feels weird, but it eliminates the potential polymorphism in the default case.

The other option is to change the headers API entirely and use a Headers object... e.g.

const headers = new http2.Headers();
headers.set(':status', 200);
headers.set('content-type', 'text/plain', { store: false });
headers.set('foo', 'bar');

This has it's whole own set of issues, including being slower than using an object literal.

@jasnell I do like the second approach, for the reason you mentioned. :) Do you think we might ever want to add more options than NGHTTP2_NV_FLAG_NO_INDEX? I don’t the NO_COPY flags would be set by userland, so we’re good on that, right?

The NO_INDEX flag is likely the only one we're going to need. I would be quite surprised if any new flag was added here.

@jasnell Prototyped this out at addaleax/node@bb2fb6c, but I’d say it’s different enough to leave it for another PR.

IMO we should go for consistent styling here (no spaces). By the way, I would actually like to enable linting against these with array-bracket-spacing: [error, never] (only 7 errors in lib/*, but quite some churn in test).

Ugh, done. I’d like to point out that, while I know arrays and objects are different things, we just enabled the opposite lint rule for objects.

I know, but as far as I know the house style is still [a, b, c]. To be honest, I personally prefer [ a, b, c ] over [a, b, c], but enforcing spacing within arrays would cause over 7400 errors within our code. But as the majority seems to prefer arrays without spaces anyway, I would go for consistency, even though @refack warned me that a PR might not be too successful, depending on the churn.

I think using V8's String API should suffice (and be preferred) here, and it doesn't have the overhead StringBytes has.

Yes – done!

Would be good to have a test case against this.

There already is :) test/parallel/test-http2-client-unescaped-path.js, if you want to have a look.

Rather inserting a fake header, can we simply abort and set an invalid flag on the class that we can check for before using the Headers instance? That way we can return a proper error code back to the js layer without having to attempt sending the headers at all. Should be faster.

@jasnell Yes, we could do that, but is that really a case that we want to optimize for? Plus, it would create inconsistency with how we handle other invalid headers.

nah, you're right, this works.

It might be appropriate to enlarge the default allocated stack size, as the size is determined by the type and a constant. nghttp2_nv is obviously bigger than a char.

I think this buffer was really oversized before. I don’t know what the typical number of headers and their sizes would be, but I’m pretty sure 1024 headers were excessive :)

If this buffer is expected to contain the full string of headers, it may need to be quite a bit larger. There are real world cases of applications sending headers with > 1 MB of data.

but yes, 1024 header pairs is.... excessive ;-) (I'd just never got around to tuning the size of it)

What about alignment? A compound type like nghttp2_nv has more strict alignment requirements than char. It will probably work out alright in practice but by accident, not by design.

If this buffer is expected to contain the full string of headers, it may need to be quite a bit larger. There are real world cases of applications sending headers with > 1 MB of data.

It is expected to be able hold the full string of headers often enough. I’ve bumped this to 3000 bytes by default, that ought to be enough for everyone ;) Even if not, the worst things that happens is a single malloc() + free() combination.

What about alignment? A compound type like nghttp2_nv has more strict alignment requirements than char.

Yeah, I guess it would always work, but I’ve added code to account for that concern now anyway.

using mapToHeaders(headers).split('\0') and keeping the second argument as an array (albeit not an array of arrays) may be easier to read

Using Array#join was a bit easier, but yes, good idea – done!