Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calls should reject promise if target window is unresponsive #61

Open
mrcnski opened this issue Apr 9, 2021 · 4 comments
Open

Calls should reject promise if target window is unresponsive #61

mrcnski opened this issue Apr 9, 2021 · 4 comments

Comments

@mrcnski
Copy link

mrcnski commented Apr 9, 2021

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
Copy link
Owner

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
Copy link
Author

mrcnski commented Apr 9, 2021

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
Copy link
Owner

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

@mrcnski
Copy link
Author

mrcnski commented Apr 16, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants