alesgenova / post-me

đŸ“© Use web Workers and other Windows through a simple Promise API
https://www.npmjs.com/package/post-me
MIT License
492 stars 13 forks source link

Calls should reject promise if target window is unresponsive #61

Open mrcnski opened 3 years ago

mrcnski commented 3 years ago

Is your feature request related to a problem? Please describe. If I call A() on a child window, the target window may become unresponsive while running A() and the caller is stuck waiting on the result of A().

Describe the solution you'd like I'd like for the call to fail after a set amount of unresponsive time (e.g. 2 seconds).

Describe alternatives you've considered I can manually implement a simple ping() method on the target and have the caller ping it periodically to check if the target window is still responsive. This is not ideal because we have a complex architecture with many target windows and we'd have to implement ping() on all of them. I think changes in post-me are required for this, I can do it myself if you don't have time but any guidance is appreciated.

We have the target windows catching errors and onunload events and those are handled properly, but there are situations in which those aren't triggered.

Additional context Related but different is a timeout parameter for calls. We don't want calls to timeout for our specific use case because we expect some calls to take an indefinite amount of time. We just want to check that the window continues to be responsive.

alesgenova commented 3 years ago

I think an initial approach could be to add an optional timeout field to the existing customCall()'s options parameter.

For example, calling the method "foo" with no parameters, would look something like this:

remoteHandle.customCall('foo', [], {timeout: 1000});

Additionally, we could introduce a new method similar to setCallTransfer(), so that the timeout parameter can be set just once per method instead of passing it with each method call. For example:

remoteHandle.setCallTimeout('foo', 1000); // Set timeout just once
remoteHandle.call('foo'); // Each call to foo will reject after 1000ms

Let me know if the approach proposed here would be enough to solve your use case, I can work on a fix later in the week (unless you would like to give it a shot!).

PS: I'm trying to understand what you mean exactly in the "Additional context" paragraph: are you suggesting for post-me to have and internal polling mechanism to verify if the other end is still responsive?

mrcnski commented 3 years ago

I'm trying to understand what you mean exactly in the "Additional context" paragraph: are you suggesting for post-me to have and internal polling mechanism to verify if the other end is still responsive?

Yes, apologies if I wasn't clear. When I see "timeout" I think: I call A(), it runs for 2 seconds, after which the call is not returned so I consider it to be a timeout. I tried to say that this behavior is specifically not what is desired (though I can see use cases for it).

The behavior I want (and I think is more useful generally for post-me comms) is: I call A(), it runs for some unspecified amount of time (say two hours) during which it is responsive but the call is not returned, but then fails to respond to polling for 2 seconds -- this is what I want to consider a "timeout". Not sure if that is the correct terminology here (I'm just getting started on the front-end side of the stack).

I like both of your suggestions (assuming "timeout" is a good term there). For our use case being able to set the "timeout" in both customCall and setCallTimeout may be useful, depending on the needs of the caller 👍

Thanks for the quick response 🙂 Once we've settled on the API, I may have time to attempt the fix myself. I'll let you know when I start so we don't duplicate work.

alesgenova commented 3 years ago

@m-cat Any updates on your end? Let me know if you run into any issues or need some help

mrcnski commented 3 years ago

Hey @alesgenova, I haven't started yet because I wasn't sure if you agreed with the suggestion. Anyway, I can try to start within a week or so, but got quite a bit on my plate atm