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

Remove JS TLS socket event listeners on finalise #3336

Conversation

mikeshepherd
Copy link

This fixes an issue we see when running this in AWS lambdas via Scala.js and feral.

The issue is that the lambdas can fail with java.lang.IllegalStateException: dispatcher already shutdown. This is coming from the event handler for an error event on a TLS socket. As this happens in a node error handler there is no way for our code to catch this exception.

For this to happen this error event must be processed by the callback after the finalizers for the TLSSocket Resource have run, as that is where the dispatchers will be shutdown.

I've extended the existing helpers for adding event listeners to provide Resource versions for once events and changed the upgrade thunk a Resource so these helpers can be used to add, and remove, the event listeners

@armanbilge armanbilge self-requested a review November 15, 2023 20:13
@mikeshepherd
Copy link
Author

I've realised there's probably an issue with this approach, and the reason that the thunk wasn't a resource in the first place, which is the explanation in ioplatform about running on the MicrotaskExecutor. I think there should be a reasonable fix for this, it'll just be a bit bigger. Hang tight while I sort it

@armanbilge
Copy link
Member

I've realised there's probably an issue with this approach, and the reason that the thunk wasn't a resource in the first place, which is the explanation in ioplatform about running on the MicrotaskExecutor.

Yes, exactly!! Good catch. I'll mark as a draft for now.

Thanks for the PR :)

@armanbilge armanbilge marked this pull request as draft November 15, 2023 20:26
@mikeshepherd mikeshepherd marked this pull request as ready for review November 15, 2023 21:41
@armanbilge
Copy link
Member

Would you be open to trying a more involved refactoring? I was never crazy about how this was working, and the problems seem to be compounding.

So here's what I propose: we introduce a new lifecycle concept of Resource[F, EventEmitterScope] with helpers like

def unsafeRegisterOneTimeListener[F[_], E](eventName: String, dispatcher: Dispatcher[F], scope: EventEmitterScope)(
    listener: E => F[Unit]
): Unit = /* register listener and add cleanup task to scope */

This way we can register multiple listeners in a single delay block and they can add their cleanup action to the EventEmitterScope. The scope would guarantee that the listeners get cleaned up without requiring that registration itself be a Resource. Then we would no longer need the evalOn(MicrotaskExecutor) hack.

@mikeshepherd
Copy link
Author

Would you be open to trying a more involved refactoring?

Yes and no.
I'm very happy to do that, though I might have to reach out separately to work out the finer details with you, but I'd like to get this merged in as well, if you are happy with it. Without this change there is the very real possibility that we'll switch our lambdas from JS to JVM, and then I'll have a lot less scope to work on a larger change.

Would you accept a promise that it'll be the very next thing I do if we get this change merged so it unblocks our lambdas sooner?

@gvolpe
Copy link
Member

gvolpe commented Nov 16, 2023

@armanbilge I can act as witness to that promise if that's any guarantee 😅

We work together with Mike and would like to continue using JS lambdas, but are close to losing the battle with this new issue.

@armanbilge
Copy link
Member

if you are happy with it

Sorry, I appreciate your work, but I don't want to merge/release this PR as-is. I understand you don't have time to work on the larger refactoring I proposed, so I did it in #3338.

@mikeshepherd
Copy link
Author

Totally understood. Thanks for getting your proposal out so quickly as well. Gonna close this out in favour of that

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

Successfully merging this pull request may close these issues.

3 participants