Add validate to Deno.read(), Deno.write()

[rusty-cat-life]

Fix of #2341

I added a validation before fireing sendAsyncMinimal()

I added

  if(!Number.isInteger(rid)) {
    throw new Error("rid must be integer.");
  }

  if(!(p instanceof Uint8Array)) {
    throw new Error("data must be Uint8Array.");
  }

for write() and read()

[CLAassistant]

All committers have signed the CLA.

[ztplz]

It doesn't seem necessary, if the wrong parameter is still used at the type prompt, it is the user's problem.

[hayd]

This should be addressed in rust.

[rusty-cat-life]

Okay, I'll close

[ry]

@rusty-cat-life I appreciate the PR and I agree that the current behavior is going to be confusing for JS users (as opposed to TS users) ... Personally I'm up in the air about whether we should add runtime checks. It seems unlikely that they will have a measurable performance impact.

We measure the performance of read() and write() carefully - because a lot of infrastructure sits on top of them.

Out of curiosity I took a look at the perf results from this branch. Only "deno" and "deno_net_http" would be effected by your change:

  "req_per_sec": {
    "node": 32197, 
    "deno_core_single": 76737, 
    "hyper": 95360, 
    "deno_core_multi": 83063, 
    "deno_net_http": 20003, 
    "deno": 70522, 
    "node_tcp": 77867
  }, 
  "max_latency": {
    "node": 12.85, 
    "deno_core_single": 8.13, 
    "hyper": 1990.0, 
    "deno_core_multi": 11.07, 
    "deno_net_http": 8.63, 
    "deno": 12.19, 
    "node_tcp": 8.19
  }, 

These numbers are quite noisy, so we can't read too much into them, but a recent commit in master branch looks like this:

  "req_per_sec": {
    "node": 31807, 
    "deno_core_single": 74753, 
    "hyper": 97024, 
    "deno_core_multi": 84515, 
    "deno_net_http": 17583, 
    "deno": 67927, 
    "node_tcp": 85105
  }, 
  "max_latency": {
    "node": 15.32, 
    "deno_core_single": 8.16, 
    "hyper": 21.82, 
    "deno_core_multi": 8.07, 
    "deno_net_http": 9.55, 
    "deno": 14.48, 
    "node_tcp": 8.18
  }, 

There is no obvious degradation by adding these.

[hayd]

This is similar to runtime checks in all Deno functions (I think this has come up before), the complaint is not runtime performance but code overhead of doing this around every function.

Rust-side should not be panicing... no matter what. That's why I think it must be fixed on the rust-side, by removing these unwraps, that way the ts-check will be redundant.

I also appreciate the PR, sorry if my comment came across as dismissive (it wasn't the intention).

[ry]

#2349 has a fix on the rust side for the panic