Custom logger func

[hoho]

As an embedder, I have a necessity to redefine the logger function (fprintf is currently used).
This change lets to customise the logger function. By default, the behaviour keeps being the same
This pull request is a follow-up for the obsolete #1468.

[bnoordhuis]

(Recurring) style error: const char* - i.e. the star should lean to the left.

[hoho]

@bnoordhuis sure, I'll fix everything today evening. Are there other concerns about the whole thing?

[bnoordhuis]

I've so far only commented on the technical implementation, not on whether it's actually a desirable feature. I'm personally neither -1 nor +1.

Aside, the commit log will need to conform to the contributing guidelines and the NODE_EXTERN functions should arguably go into src/node.h.

[hoho]

@bnoordhuis ok, I've fixed the technical things you've mentioned. Hopefully will have enough +1s to fix the rest.

[hoho]

I've tested it on OSX, Windows 10 and Linux.

[MylesBorins]

@hoho while I would not be anywhere close to the final say on this commit, I think it would be hard for new functionality like this to land if it did not come with updated docs explaining the new functionality.

[jbergstroem]

CI: https://ci.nodejs.org/job/node-test-commit/1624/

[jbergstroem]

This looks like an excellent opportunity to expand our c++ unit tests if we decide the change belongs in core.

[hoho]

Ah, I tested Windows before adding attribute. I'll fix it.
I can do unit tests and docs as well. As soon as I'm confident they are not meaningless waste of time :).

[jasnell]

Cautiously marking as semver-major. minor might be more appropriate tho

[Fishrock123]

I'm not 100% sure (c++) but this looks semver-minor to me. Correct me if I'm wrong but it looks ABI-compatible.

[jasnell]

Any further updates on this one?

[hoho]

I still need it and ready to rebase/fix/update.

[jasnell]

@hoho .. look like this fell through the cracks... likely because no one noticed your last rebase. If this is still something you'd like to do, can you please rebase and squash your commits again?

[hoho]

I still need it, I'll rebase/squash.

[jasnell]

ping @hoho ... still want to pursue this?

[hoho]

I have given my initial approach more thought and I have a better one.
Basically all I need as an embedder is to be able to redefine the print functions. I've created print.h header which defaults all the functions to their standard implementations, but I will be able to create my custom print.h.

Hopefully, everybody will like it better than the previous one. The best part is that it literally changes nothing for all the users.

[hoho]

@fhinkel ping :).

[fhinkel]

CI: https://ci.nodejs.org/job/node-test-pull-request/7174/

[hoho]

Looks like something (not connected with this PR) was broken. Plus I've fixed the missing newline.

[BridgeAR]

Is this stalled?

[gibfahn]

@hoho could you rebase one more time?

[hoho]

Sure...

[bnoordhuis]

Since this has conflicts and seems to have been stalled for a year-and-a-half now with no LGTMs, I'm going to close it out. Thanks for the PR though.

[hoho]

@bnoordhuis Amazing. I've been rebasing it several times, I've changed the approach (and nobody made a single comment about it, by the way, I guess nobody has checked it out).
So, I'll create a new PR, because I still need it.

[gibfahn]

@hoho I think maybe you forgot to push after the last rebase, or something else went wrong, Github was still showing merge conflicts after my last comment. Sorry for forgetting this, I should have come back to it. Feel free to ping me on Twitter if you comment on the issue and people don't respond.

The bigger issue here is that no collaborators have approved or said that they're +1 on this (either in here or in #1468). @nodejs/collaborators would anyone be willing to review this? AFAICT it just replaces printf, fprintf, vfprintf, and fflush with a macro so they can be redefined, so technically speaking it's a pretty simple change.

Looking at the history of the files touched, maybe @bnoordhuis @jasnell @sam-github @addaleax @danbev .

I think I agree with @Fishrock123 (#4523 (comment)) that this is semver-minor rather than major. Adding a new header file shouldn't break anyone right?

[refack]

From a quick glance it seems like a good idea. I'll give it look tomorrow. I also think it's semver-minor, if not patch.
It does however only solves custom build embedding, it doesn't solve static/dynamic library embedding AFAICT (but I can't think of a non semver-major way to solve that).

[addaleax]

It does however only solves custom build embedding, it doesn't solve static/dynamic library embedding AFAICT (but I can't think of a non semver-major way to solve that).

We could just use and expose function pointers (or, preferably, a single struct with function pointers/virtuals) instead of the static stdlib function names. That would still be semver-minor and solve the problem just as well, plus it would make this actually testable.

[seishun]

We could just use and expose function pointers (or, preferably, a single struct with function pointers/virtuals) instead of the static stdlib function names. That would still be semver-minor and solve the problem just as well, plus it would make this actually testable.

It would also make it slower than a simple function call.

[bnoordhuis]

Slower and with more room for error in case the function pointer gets clobbered.

What's more, fprintf is a weak symbol that you can overload, or you can make stdout and stderr point somewhere else: int fd[2]; pipe(fd); stderr = fdopen(fd[1], "w");

Since solutions exist that don't encumber the code base I don't really see a reason to land this change.

[gibfahn]

Since solutions exist that don't encumber the code base I don't really see a reason to land this change.

You mean monkey-patching fprintf (as @hoho is/was doing in #1468 (comment))?

[bnoordhuis]

Maybe. It isn't clear to me what "monkey-patching" means there.

[benjamingr]

I don't see anything wrong with piping stdout either - it's very common practice to redirect stdout somewhere when composing applications. An embedder already has a hook at that level and can use it.

I'm -1 on landing this as is unless a compelling technical argument can be made for it.

[benjamingr]

Making -1 explicit.

[gireeshpunathil]

I guess custimization here would mean more than / other than just redirecting. An embedder can implement a custom variadic function and write the data in its own style, apart from routing to its own destination. May be these are two different things, and redirection as a sole purpose for the embedder materialized into the PR over an year of dormancy?

Not to rebuke points made in the above reviews, but I guess operating at functional abstractions is relatively cleaner than operating at the file descriptor level. While I agree that the routing logic alone can be achieved without changing existing node code, the code base itself contains examples of symbol overriding, for node's internal purposes.

If there is a consensus on embedders being a decent and valid use case, this change seems reasonable, structured and maintainable.

[hoho]

I might try to describe the exact use case, in order to make it more straightforward.

I have a very slowly progressing project called dosido. It's nginx and Node.js in one binary. It's feature incomplete, but I'm using it in a couple of personal projects and still planning to get it to a production-ready state some day.

What I need is to log all Node.js output using nginx logger functions. I build Node.js as a static library and I'm customizing node.gyp (to exclude the original node.cc and add some more files) and node.cc (to hook into the Node.js event loop). Currently I'm redefining fprintf. And it works, but feels like an ugly workaround and plus one change I need to put to my node.cc every time I upgrade the version. I guess I can live with it, I just wanted to use features not workarounds.

Hearing about piping it somewhere else makes me cry a little when I think of Windows.

[hoho]

Having project personalized output functions is a thing I've met more than once. On top of my mind in V8 itself.

[jasnell]

@nodejs/ctc ... any further thoughts on this?

@hoho ... with the current objections this is not able to land. If enough @nodejs/ctc members feel strongly about it we can revisit it. I know that's no the outcome you were hoping for and I'm sorry for the effort you've put into this. I certainly hope you won't be too discouraged from contributing in other ways. Perhaps there's another way this issue could be tackled?

Either way, thank you for working through the process with us.

[benjamingr]

Thank you for your contribution. Since there hasn't been discussion for a while I'm going to close that now - but if you'd like to pursue this further and feel like there is new information please let me know and I'll reopen it.