Details
Diff Detail
Event Timeline
To be honest, I'm not sure if "event" is the best name for it. I get it's a WSA name but it's a bit non-obvious to outsiders (I mean, technically everything is an event). Not that I have a better name in mind.
lldb/include/lldb/Host/MainLoop.h | ||
---|---|---|
66 ↗ | (On Diff #449941) | Could you include a short comment explaining how it's used? I suppose something like "will be executed upon being explicitly signaled via NotifyEvent() from any thread". |
lldb/source/Host/common/MainLoop.cpp | ||
121 ↗ | (On Diff #449941) | I think the reliance on < vs <= here could be confusing. Perhaps add a note that we popped the "event event" above, or maybe add a last_event const-var prior to popping it. |
133 ↗ | (On Diff #449941) | Let's make sure something didn't corrupt the state. |
Yeah, I know... I checked the thesaurus for "event", and I didn't find anything reasonable. And "signal" is obviously a bad idea..
Another possibility (one that just occurred to me right now) would be to use a completely different paradigm to achieve the effect. For example, we could extend the PendingCallback mechanism to enable adding callbacks from different threads (and have them wake the loop if necessary). That would be a more generic mechanism, and it would avoid the need to introduce a new concept (and having to name it).
That would be appreciated, but I think we first need to figure out the API, and how this code is going to be structured (I'm going to try handling the second part now)...
Rebase on top of D131159 and partially implement the Linux counterpart.
Note that I have only covered sys/event.h for now and that I haven't tested the updated Windows code.
Ok, I've just realized that the pipe writing solution isn't going to work when the notification happens from the same thread (as the unittest does right now). @labath, do you happen to have an idea how to solve that reasonably cleanly?
Ah, nevermind, for some reason I thought pipe writes are going to block until read happens. Perhaps we don't need to worry about that.
Thanks for taking care of the posix part.
I'm sort of warming up to the idea of reusing pending callback machinery for this. Besides avoiding the awkward naming problem, it also avoid another somewhat strange situation where our EventHandle offer additional actions on the event (which is different from all our other handle types, which only support deletion). All it would take is to add some locking around the PendingCallback manipulations, and write to the pipe (or trigger the event) whenever a new callback is registered, so that the Run function can invoke it.
It's a slightly different programming paradigm, and I suppose it also depends on which mechanism would be more natural for our InterruptRead use case (have you tried implementing it using this?), but I think the two mechanisms should be equivalent (as in, one could implement one using the other -- fairly easily).
What do you think about that?
A pipe has a fixed (on linux, it's configurable) capacity, and it should be sufficiently large that we don't have to worry about it. If we wanted to be super safe, we could make the writes non-blocking. It should be safe to ignore EAGAINs in this situation, as that means the pipe already contains some unprocessed bytes, and all of the pending notifications will be processed once the reading thread "catches up".
lldb/source/Host/posix/MainLoopPosix.cpp | ||
---|---|---|
229 | I like this. |
To be honest, I don't have a strong opinion, so I leave the decision to you. I was also thinking about this but decided to avoid changing the paradigm while rebasing/finishing the existing code, especially that I can't test it on Windows. But I can do it if you like.
It's a slightly different programming paradigm, and I suppose it also depends on which mechanism would be more natural for our InterruptRead use case (have you tried implementing it using this?), but I think the two mechanisms should be equivalent (as in, one could implement one using the other -- fairly easily).
Actually, I've started experimenting on rewriting the read thread option in Communication using this (with the goal of using the thread both for reads and writes).
lldb/source/Host/posix/MainLoopPosix.cpp | ||
---|---|---|
229 | Thanks. I definitely like it more than doing it three times too ;-). |
Ok, so I'm going to give it a shot today. Could you clarify one thing though?
All it would take is to add some locking around the PendingCallback manipulations
Are you thinking of locking directly in MainLoop classes, or in the caller? It'd feel a bit inconsistent if MainLoops provided locking around pending callback manipulations and not other methods. However, I suppose making everything thread-safe wouldn't hurt either.
I'm not sure this can be done from the outside, as the callback adittions need to synchronize with the accesses that happen inside the Run function. It is somewhat inconsistent, but for example python has special call_soon/call_soon_threadsafe versions for code which needs to schedule callbacks from other threads. We could do something like that. Or we could also just lock everything...
@labath, what do you think of this?
Also I'm wondering about error handling. Should we introduce fallible constructor for this?
What's the reasoning behind TriggerPendingCallbacks? I was assuming that the addition of a callback would cause it to run automatically...
I'm not particularly worried about this. I might even make the Pipe constructor non-fallible, as the only way that can fail is if we exhaust our file descriptor limit -- and I very much doubt our ability to recover from that.
In that case, is there something more I should do about this patch or are you going to take over from here?
It wasn't clear to me whether you intend to finish this patch (and I was waiting on your response to this). That said, if you don't have any strong opinions there, then I can try to finish this.
To be honest, I didn't think about it much. The original idea is that we can either have "non-important" callbacks that will be run at some point in the future, and "important" callbacks that should be run ASAP. However, I'm not really married to this idea, so I can make them triggered immediately if you prefer that.
Ah, sorry, missed that comment. I can change that logic if you prefer. Otherwise, I think the next thing that needs to be here is testing/fixing the Windows code, and I'd be grateful if you could do that.
I don't think we need to make that distinction.
Ah, sorry, missed that comment. I can change that logic if you prefer. Otherwise, I think the next thing that needs to be here is testing/fixing the Windows code, and I'd be grateful if you could do that.
I've fixed the windows version, and I've removed TriggerPendingCallbacks (from the public interface).
I think this should be good to go now.
Are you sure about that? https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46639 is just running the commit now. I'd actually suspect the breakage is caused by D133181.
I'm wondering whether I should be repeating this all over the place or perhaps just adding m_event_pipe to m_read_fds.