fix nodejs resource functions (ie registerResource) to properly propagate errors

[EvanBoyle]

Fixes #6643

More detail in the issues above, but TLDR is that we were swallowing errors in registerResource. This change fixes prepareResource to set up reject handlers on ID and URN promises to appropriately propagate the errors.

[EvanBoyle]

The original repro that lead me to discover this. Unforunately it takes ~5 mins to run and only fails ~80% of the time: #6645

[pgavlin]

Can we set up a test for this using mocks? Throwing from within newResource should work, I think.

[EvanBoyle]

@pgavlin wouldn't that just be testing the mocked registerResource implementation? If so that wouldn't really validate this fix.

I had a hard time coming up with a way to test this. The only repro I currently have is the one linked here #6645 and it takes 5 minutes to fail, and the failure is non-deterministic as it seems related to resource exhaustion at some layer.

[EvanBoyle]

(the mocked registerResource impl does't call prepareResource which is the layer at which the fix was made)

[justinvp]

(the mocked registerResource impl does't call prepareResource which is the layer at which the fix was made)

When using a mock, prepareResource should still be called...

The Resource constructor calls registerResource in runtime/resource.ts:

pulumi/sdk/nodejs/resource.ts

Line 342 in ff2cf70

registerResource(this, t, name, custom, remote, urn => new DependencyResource(urn), props, opts);

registerResource in runtime/resource.ts calls prepareResource:

pulumi/sdk/nodejs/runtime/resource.ts

Line 284 in ff2cf70

const resopAsync = prepareResource(label, res, custom, props, opts);

And then call's the monitor's registerResource (whether monitor is an actual gRPC monitor or a mock monitor):

pulumi/sdk/nodejs/runtime/resource.ts

Line 340 in ff2cf70

(monitor as any).registerResource(req, (rpcErr: grpc.ServiceError, innerResponse: any) => {

[EvanBoyle]

@justinvp ah, thanks for the pointer. I did forgot that this was mocking the monitor and not the exported calls in runtime. I'll work on getting these tests added.

[EvanBoyle]

@pgavlin @justinvp added the test. please take another look.

[justinvp]

Do we need to do something similar for the resolvers for all the other outputs in transferProperties?

[EvanBoyle]

Do we need to do something similar for the resolvers for all the other outputs in transferProperties?

This is a very good point. These are actually the most common outstanding promises that we see leaked, and that would make sense as we aren't properly rejecting them...

Fix incoming.

[komalali]

Huh, looks like the transferProperties changes are causing unhandled rejections in the tests now.

EDIT:
NVM it failed on #6561 - the stack traces in the node tests threw me off.