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

feat(flushguard): provide interface to close trace manually #10

Closed
wants to merge 1 commit into from

Conversation

kwonoj
Copy link

@kwonoj kwonoj commented Apr 4, 2022

This PR attempt to provide a way manually flush & close subscribers via close() in flushguard.

This wouldn't be required in normal cases - dropping flushguard, or flush would do the work suffeciently. However there are some edge cases lifecycle of flush is not gracefully controllable in a process lifecycle. The problem I encountered is when subscriber is initialized in node.js's napi context, and if parent process (node.js) forcefully closes via process.exit() which doesn't gaurantee to call any cleanup for the native addons. (ref: vercel/next.js#35803 (comment)).

To workaround addon passes over references to flushguard to v8's context and pass it back for the manual teardown, then call close to complete closes subscriber explicitly. Since napi passes references to the flushguard manual drop wasn't the way to go either.

There might be some idomatic way I wasn't aware of and happy to either fix up the PR or close. At least featurewise, close() seems to work as expected.

@thoren-d
Copy link
Owner

I wonder if on your end you could wrap the FlushGuard as a Cell<Option<FlushGuard>> as I did with the JoinHandle, then instead of calling FlushGuard.close() you can take() the FlushGuard out and let it be dropped.

I think it makes for a better API if you can't get a FlushGuard that doesn't work, which is what close() gives you. If I were to add more methods in the future, they could all fail after a FlushGuard has been close()d.

@thoren-d
Copy link
Owner

I'm going to go ahead and close this and suggest wrapping the FlushGuard as above if a manual close is needed. Let me know if this doesn't work for you.

@thoren-d thoren-d closed this Jul 26, 2022
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.

None yet

2 participants