fix: trustless-gateway sessions handle aborts properly #776
Conversation
|
@achingbrain open to other ideas on how to handle this. There are tests now to confirm it's being handled, but you can see in the abort signal delays that there is a race condition if the block is found within ~30ms of the abort signal firing. |
| // Note that there is a slight race condition here where we find the block and the signal is aborted within ~10s of ms of each other. | ||
| // In that case, the request will be rejected with the signal reason. |
There was a problem hiding this comment.
I'm confused by this comment - are you saying that we've found the block but we end up rejecting the promise anyway? This sounds like incorrect behaviour that should be fixed?
OTOH if the abort signal fires, but the block comes in afterwards, that's fine because the user aborted the request so they are no longer interested in the block.
the block and the signal is aborted within ~10s of ms of each other
Do you mean 10s or 10ms?
There was a problem hiding this comment.
I'm confused by this comment - are you saying that we've found the block but we end up rejecting the promise anyway? This sounds like incorrect behaviour that should be fixed?
I could have sworn that when logging in my tests, with a small ms variation, I saw that one provider logged "got block" and then the abort came through, and the whole thing was aborted.
That should not be possible, and might be a bug in this implementation, or queue, or... idk. let me confirm again.
Do you mean 10s or 10ms?
I meant "tens of milliseconds". It's slightly confusing with the shorthand, my bad.
Co-authored-by: Alex Potsides <alex@achingbrain.net>
I think the problem here is we're not rejecting the deferred promise when the user aborts the request. The signal is passed to all async operations, the intention there was for rejections to bubble up to the caller, but we ignore queue errors to allow trying other providers so nothing happens when the signal fires. I've opened #778 targeting this PR - instead of adding more conditional logic it just rejects the promise and clears the queue if the signal fires. Let me know what you think. |
If the user aborts the request, reject the promise
I like it. Short and sweet, makes sense. |
We originally did not abort when the session was idle because a block may have been found.
This PR changes that so that we have a race condition on signal.abort and foundBlock, but I believe this small window is a better situation than having the session never end (as in #775)