lucab / libsystemd-rs

A pure-Rust client library to work with systemd
https://crates.io/crates/libsystemd
Other
105 stars 19 forks source link

Re-use socket for journald messages #87

Closed swsnr closed 2 years ago

swsnr commented 3 years ago

Hello @lucab

currently the logging::journald_send always creates a fresh unbound socket to send the journald datagrams which in my understanding allocates and closes a fresh file descriptor for every journald_send call. File descriptors aren't too efficient in the kernel, so I think we should avoiding creating them frequently.

I'd like to add an API which lets me create the socket once and reuse it for all subsequent messages in my logger implementation. Specifically, I'd like to add

We can then either remove the old journal_send and journal_print, or alternatively re-implement them on to of JournalWriteClient.

I'm not sure about the JournalWriteClient name; I also considered JournalWriter, but this seems to suggest that it implements std::io::Write, or JournalClient but that might suggest that it can also read from the journal.

What do you think?

lucab commented 3 years ago

Overall I think it makes sense to re-use a socket once created. For comparison, the other implementation I have (in Go) does autobind and cache the socket on startup: https://github.com/coreos/go-systemd/blob/v22.3.2/journal/journal_unix.go#L56-L58. We could even consider doing the same here, so that consumers don't have to deal with their own caching.

swsnr commented 3 years ago

So I'd add a struct around the socket and expose, so that users can care for this if they want to, and then put a shared instance behind e.g. lazy_static for the public functions? Does this sound reasonable?

lucab commented 3 years ago

I think https://docs.rs/once_cell is the modern way. I've heard there is also some movement around having this kind of primitives in the stdlib too. You may want to look for std::lazy or similar, I'm not up-to-date on that front.

Do you have some usecases where you need to setup/retrieve the underlying connection in a consumer? If not, then we could continue keeping it internal, at least for the moment. That's what I have in the Go equivalent too, and it worked fine so far.

swsnr commented 3 years ago

You may want to look for std::lazy or similar, I'm not up-to-date on that front.

I'm even less up to date, I didn't even know about once_cell :innocent:

Do you have some usecases where you need to setup/retrieve the underlying connection in a consumer?

Use cases, per se, well… it's not as if not having it exposed was a show stopper :slightly_smiling_face:

But I find a struct around the socket an obvious abstraction which I'd have added anyway because I find it clearer, so why not expose it?

And then, log for instance already keep state in terms of its shared global logger, so we cloud simply rely on this, by wrapping the (indirectly) exposed socket into a Log object and rely on the global state already provided by logger, instead of adding another piece of global state on our own.

And ultimately I'm just not so much a fan of hidden global state, as a matter of personal preference :innocent:, and I also like libraries which open up their internals in reasonable abstractions which I can re-compose at my liberty more than libraries which strictly hide their internals. :slightly_smiling_face:

But it's your choice, if you'd prefer I'd not be fundamentally opposed :bow: I'd be disappointed tho :pensive:

lucab commented 3 years ago

I'm open to see how the result looks like, let's maybe split such PR into multiple commits so that it's easier to iterate on. The trade-offs are related to increased complexity and UX friendliness. I'd still expect most people to use the current journal_send and journal_print, especially when writing libraries.

swsnr commented 3 years ago

@lucab I see your point. I'll make a merge request soon'ish :slightly_smiling_face: