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

fix: forward dispatch return value #3368

Merged
merged 4 commits into from
Jun 27, 2024
Merged

fix: forward dispatch return value #3368

merged 4 commits into from
Jun 27, 2024

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 25, 2024

Refs: #3373
Refs: #3370

@ronag ronag requested a review from metcoder95 June 25, 2024 07:33
@ronag ronag force-pushed the forward-dispatch branch 2 times, most recently from 1512421 to 1d160da Compare June 25, 2024 07:39
@ronag ronag force-pushed the forward-dispatch branch from 1d160da to 766c3b8 Compare June 25, 2024 07:43
@ronag ronag marked this pull request as draft June 25, 2024 09:08
ronag added a commit that referenced this pull request Jun 25, 2024
@ronag ronag mentioned this pull request Jun 25, 2024
@ronag
Copy link
Member Author

ronag commented Jun 25, 2024

Things this doesn't solve.

  • async dispatch
  • drain event parameters (which you could argue are useless)

@ronag ronag marked this pull request as ready for review June 25, 2024 18:11
Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has sense; I just feel that this can cause the false sensation of having a fully fledge dispatcher.

It might be possible that we end with max listeners problems.

Shall we tests the events are emitted as we expect?

@ronag
Copy link
Member Author

ronag commented Jun 25, 2024

I feel this is just a hack until we have a better solution.

@ronag
Copy link
Member Author

ronag commented Jun 25, 2024

We would need to have some form of weak event listeners for this to work well.

@ronag
Copy link
Member Author

ronag commented Jun 25, 2024

@benjamingr Do you have any input on how we can have weak event listeners here that when the ComposedDispatcher is gc:d will be freed?

@ronag ronag force-pushed the forward-dispatch branch 2 times, most recently from f1e14a3 to 8855481 Compare June 25, 2024 18:41
@ronag ronag force-pushed the forward-dispatch branch from 8855481 to 67de892 Compare June 25, 2024 18:44
@ronag
Copy link
Member Author

ronag commented Jun 25, 2024

This is still not great though... until GC the event listeners are still registered.

@ronag
Copy link
Member Author

ronag commented Jun 25, 2024

The alternative is that we have some kind of ref/unref API. But that is going to be a constant pain of people doing it wrong.

ronag added a commit that referenced this pull request Jun 25, 2024
ronag added a commit that referenced this pull request Jun 25, 2024
ronag added a commit that referenced this pull request Jun 25, 2024
ronag added a commit that referenced this pull request Jun 25, 2024
ronag added a commit that referenced this pull request Jun 25, 2024
@benjamingr
Copy link
Member

@benjamingr Do you have any input on how we can have weak event listeners here that when the ComposedDispatcher is gc:d will be freed?

We don't currently expose it but you can use kWeakHandler in the version bundled with Node, that lets you pass a listener and an object that the listener's lifetime is bound to - that's EventTarget though.

So we would need to build kWeakHandler (or similar) to EventEmitter and expose that to undici somehow - it has to be baked into the abstraction (EventEmitter) itself - otherwise it leaks the WeakRef itself (as you probably noticed).

@ronag ronag requested review from metcoder95 and mcollina June 26, 2024 05:33
@ronag
Copy link
Member Author

ronag commented Jun 26, 2024

@metcoder95 @mcollina wdyt of returning a Proxy?

ronag added a commit that referenced this pull request Jun 26, 2024
@metcoder95
Copy link
Member

So we would need to build kWeakHandler (or similar) to EventEmitter and expose that to undici somehow

This sounds great, but personally I'm afraid we will be attaching undici to Node internals and userland usage might suffer from a workaround that does not behaves the same.
It would be nice to have something we can use in the form of a public API for that

wdyt of returning a Proxy?

It simplifies a lot certainly, how does the gc behave (out of curiosity mostly)?

@ronag
Copy link
Member Author

ronag commented Jun 26, 2024

how does the gc behave (out of curiosity mostly)?

We cannot say anything definite about gc behavior. It's totally undefined.

@ronag ronag merged commit 3fd1fe1 into main Jun 27, 2024
36 checks passed
@Uzlopak Uzlopak deleted the forward-dispatch branch August 1, 2024 09:57
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
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