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

Raise an exception if an event signal notifies a failure state on EvtSubscribe #44

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

cosmo0920
Copy link
Member

@cosmo0920 cosmo0920 commented Mar 1, 2024

With event signal system, we're able to check the status of the event subscriptions.
This could be helpful for detecting failure of the subscribed handles.


This pull type of subscription,
we need to add the following actions:

  1. Create an EventObject for handling signal events
  2. Reset signal when ERROR_NO_MORE_ITEMS from EvtNext

This should ensure the windows eventlog collection correctly.
Also, we shouldn't specify the INFINITE waiting on WaitForSingleObject
because this function is inside of enumerator.
So, it causes infinite blocking for this case.

@cosmo0920 cosmo0920 requested review from ashie, kenhys and daipom March 1, 2024 07:04
@cosmo0920 cosmo0920 marked this pull request as draft March 1, 2024 07:17
@cosmo0920 cosmo0920 closed this Mar 1, 2024
@cosmo0920 cosmo0920 reopened this Mar 1, 2024
@cosmo0920 cosmo0920 force-pushed the wait-signal-event-correctly branch from 60bd65e to 43148f3 Compare March 1, 2024 07:44
@cosmo0920 cosmo0920 marked this pull request as ready for review March 1, 2024 08:06
From this pull type of subscription,
we need to add the following actions:

1. Create an EventObject for handling signal events
2. Reset signal when ERROR_NO_MORE_ITEMS from EvtNext

This should ensure the windows eventlog collection correctly.
Also, we shouldn't specify the INFINITE waiting on WaitForSingleObject
because this function is inside of enumerator.
So, it causes infinite blocking for this case.

ref: https://learn.microsoft.com/en-us/windows/win32/wes/subscribing-to-events#pull-subscriptions

Signed-off-by: Hiroshi Hatake <[email protected]>
@daipom
Copy link

daipom commented Mar 5, 2024

rb_winevt_subscribe_next is a function that enumerates all the current data, right?
Then, I don't see the need to wait for the signal as in this example.

(From #44 (comment))

Sorry if I'm not understanding it correctly.
I still don't understand why we need to wait for this signal.

I don't see a problem with the current design, where rb_winevt_subscribe_each_yield iterates all the current data until ERROR_NO_MORE_ITEMS occurs.

If we implement it as in this example, shouldn't we add a new feature to pass a callback and have it keep listening?

@cosmo0920
Copy link
Member Author

cosmo0920 commented Mar 5, 2024

If we implement it as in this example, shouldn't we add a new feature to pass a callback and have it keep listening?

This is because we didn't checked the internal state of subscriptions. The current mechanism is just passing no working signal into EvtSubscribe and no one checked the result.

@cosmo0920
Copy link
Member Author

cosmo0920 commented Mar 5, 2024

I still don't understand why we need to wait for this signal.

The current behavior of around the signal is:
CreateEvent(NULL, FALSE, FALSE, NULL) is not acknowledged for the state change for EvtSubscribe.
The expected behavior is:
Creating an event signal with CreateEvent(NULL, TRUE, TRUE, NULL) and respond and to be checked for the internal state changes of EvtSubscribe.

CreateEventA reference: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-createeventa

@cosmo0920
Copy link
Member Author

@daipom Are my comments answered your question? Thanks for your comment.

@daipom
Copy link

daipom commented Mar 5, 2024

@cosmo0920 Thanks for your explanation!

This is because we didn't checked the internal state of subscriptions. The current mechanism is just passing no working signal into EvtSubscribe and no one checked the result.

In the current design, it looks like it is unnecessary to check it.
Isn't it sufficient to just care that ERROR_NO_MORE_ITEMS occurs?

The expected behavior is:
Creating an event signal with CreateEvent(NULL, TRUE, TRUE, NULL) and respond and to be checked for the internal state changes of EvtSubscribe.

I'm wondering if this is really the expected behavior in the current design.

In this example, it is designed to call a function in response to event notifications.
It is different from our current implementation.

@daipom
Copy link

daipom commented Mar 5, 2024

In this example, it is designed to call a function in response to event notifications.
It is different from our current implementation.

The example needs to handle the signal in order to respond to the state.

On the other hand, rb_winevt_subscribe_each_yield just reads all the data at that point.
Therefore, in_windows_eventlog2 uses a timer event.

https://github.com/fluent/fluent-plugin-windows-eventlog/blob/e061144738ddd3c54971a6966d73a98335ae473f/lib/fluent/plugin/in_windows_eventlog2.rb#L251-L259

It would be good to use a signal like this example so that it can respond to the state without using a timer.
However, isn't that a different story than the implementation of rb_winevt_subscribe_each_yield?

@ashie
Copy link
Member

ashie commented Mar 5, 2024

This is because we didn't checked the internal state of subscriptions. The current mechanism is just passing no working signal into EvtSubscribe and no one checked the result.

In the current design, it looks like it is unnecessary to check it. Isn't it sufficient to just care that ERROR_NO_MORE_ITEMS occurs?

To tell the truth, I have same impression on this pull request.
Probably it doesn't make sense.
But I'm not sure because the implementation of Windows isn't open.
We may actually be able to find out some details about an error by this change.
So I don't object to this change.

@daipom
Copy link

daipom commented Mar 5, 2024

This is because we didn't checked the internal state of subscriptions. The current mechanism is just passing no working signal into EvtSubscribe and no one checked the result.

In the current design, it looks like it is unnecessary to check it. Isn't it sufficient to just care that ERROR_NO_MORE_ITEMS occurs?

To tell the truth, I have same impression on this pull request. Probably it doesn't make sense. But I'm not sure because the implementation of Windows isn't open. We may actually be able to find out some details about an error by this change. So I don't object to this change.

I don't want the code to be unnecessarily complex, but if it is expected to have advantages such as being able to detect errors that have been overlooked so far with raise_system_error(rb_eSubscribeHandlerError, GetLastError()), then it would be good.

@cosmo0920
Copy link
Member Author

cosmo0920 commented Mar 5, 2024

This is because we didn't checked the internal state of subscriptions. The current mechanism is just passing no working signal into EvtSubscribe and no one checked the result.

In the current design, it looks like it is unnecessary to check it. Isn't it sufficient to just care that ERROR_NO_MORE_ITEMS occurs?

To tell the truth, I have same impression on this pull request. Probably it doesn't make sense. But I'm not sure because the implementation of Windows isn't open. We may actually be able to find out some details about an error by this change. So I don't object to this change.

I don't want the code to be unnecessarily complex, but if it is expected to have advantages such as being able to detect errors that have been overlooked so far with raise_system_error(rb_eSubscribeHandlerError, GetLastError()), then it would be good.

I wondering if we could detect internal error of EvtSubscribe. In push subscription case, the registered callback in EvtSubscribe will return failure state with: EvtSubscribeActionError and not ERROR_EVT_QUERY_RESULT_STALE status.
Also, the example displays the error handlings for internal failed state in pull subscription case as I mentioned earlier.

With the current implementation is 99% sure of the usecase but high frequently flowing rate environment could generate such stales from invalid statuses so I wanted to capture them.

@daipom
Copy link

daipom commented Mar 5, 2024

With the current implementation is 99% sure of the usecase but high frequently flowing rate environment could generate such stales from invalid statuses so I wanted to capture them.

I see. Now, I understand the point. Thanks.
Then, it would be good if it does not affect the operation in the normal condition.

Is it safe to do ResetEvent for every EvtNext?
In the example, it is done for every ERROR_NO_MORE_ITEMS.
I'm slightly concerned about this difference.
I'm concerned that some data might be left unread.

@cosmo0920
Copy link
Member Author

cosmo0920 commented Mar 5, 2024

Is it safe to do ResetEvent for every EvtNext?
In the example, it is done for every ERROR_NO_MORE_ITEMS.
I'm slightly concerned about this difference.
I'm concerned that some data might be left unread.

I intended the following procedure here:

When the EvtNext function fails and sets the last error to ERROR_NO_MORE_ITEMS, reset the event object and wait for the service to signal the object again when there are events in the result set.

So, ERROR_NO_MORE_ITEMS is a key for starting to be acknowledged again and the event object should be changed to be signaled in the latter loops.

@daipom
Copy link

daipom commented Mar 5, 2024

Is it safe to do ResetEvent for every EvtNext?
In the example, it is done for every ERROR_NO_MORE_ITEMS.
I'm slightly concerned about this difference.
I'm concerned that some data might be left unread.

I intended the following procedure here:

When the EvtNext function fails and sets the last error to ERROR_NO_MORE_ITEMS, reset the event object and wait for the service to signal the object again when there are events in the result set.

So, ERROR_NO_MORE_ITEMS is a key for starting to be acknowledged again and the event object should be changed to be signaled in the latter loops.

Oh, I'm sorry.
I was misreading the code.

ResetEvent is not called for every EvtNext.
It is called only when ERROR_NO_MORE_ITEMS.

However, I still have concerns about the timing of doing WaitForSingleObject and ResetEvent.

For example, WaitForSingleObject is called for every rb_winevt_subscribe_next.
However, the event will remain signaled until reset because bManualReset is TRUE,
Does this make sense?

As @ashie says, we can't know the implementation of Windows, so it is fine that we can possibly get an error with this though it may not make sense.
In that case, we should leave a comment to the WaitForSingleObject.

As a side note, I wonder if it should be done at the rb_winevt_subscribe_each level, as follows.

static VALUE
rb_winevt_subscribe_each(VALUE self)
{
  RETURN_ENUMERATOR(self, 0, 0);

  {Confirm the event}

  while (rb_winevt_subscribe_next(self)) {
    rb_ensure(
      rb_winevt_subscribe_each_yield, self, rb_winevt_subscribe_close_handle, self);
  }

  {Reset the event}

  return Qnil;
}

@cosmo0920
Copy link
Member Author

cosmo0920 commented Mar 5, 2024

For example, WaitForSingleObject is called for every rb_winevt_subscribe_next.
However, the event will remain signaled until reset because bManualReset is TRUE,
Does this make sense?

It is totally not making sense.
Because a subscription via EvtSubscribe is not guaranteed for the further processing result. That internal state is only revealed via signal event.
Currently, we're completely ignoring their states and lack of checking whether normally processed or not.
Ensuring 99.5% or more to collect Windows EventLogs, we need to check their invalid state.

In that case, we should leave a comment to the WaitForSingleObject.

We should leave a comment for this side note instead of the proposed each loop level of acknowledged and reset manually the signal.

However, the event will remain signaled until reset because bManualReset is TRUE,
Does this make sense?

This is the real reason what I set up to create with bManualReset is TRUE. Otherwise, we didn't recognize the internal invalid state through the signal. EvtSubscribe's SignalEvent agrument requests to register a signal event which is created with bManualReset and bInitialState are TRUE.

@cosmo0920 cosmo0920 force-pushed the wait-signal-event-correctly branch from 801f1d9 to f9f833d Compare March 5, 2024 09:34
@daipom
Copy link

daipom commented Mar 6, 2024

Could you please change the PR title to make it clear the purpose?
Such as Capture error from signal event on Subscribe.

@cosmo0920 cosmo0920 changed the title Wait signal event correctly on Subscribe Raise an exception if event signal notifies a failure state Mar 6, 2024
@cosmo0920 cosmo0920 changed the title Raise an exception if event signal notifies a failure state Raise an exception if an event signal notifies a failure state Mar 6, 2024
@cosmo0920 cosmo0920 changed the title Raise an exception if an event signal notifies a failure state Raise an exception if an event signal notifies a failure state on EvtSubscribe Mar 6, 2024
@cosmo0920
Copy link
Member Author

cosmo0920 commented Mar 6, 2024

I changed this PR title. Is there anything to be needed things to fulfill to get merged?

@daipom
Copy link

daipom commented Mar 6, 2024

LGTM. Thanks!

@cosmo0920
Copy link
Member Author

@ashie @daipom If you have a cycle, could you merge this PR and make a new release to publish this change? As @daipom, pointed that the signal is mainly used for checking existence of pulled events. Not for indicating that there is an error. Thanks for this careful your insights. You guys rock!

@ashie ashie merged commit 82d567c into master Mar 6, 2024
8 checks passed
@ashie ashie deleted the wait-signal-event-correctly branch March 6, 2024 03:13
@ashie
Copy link
Member

ashie commented Mar 6, 2024

I'm going to release it as v0.10.2, OK?

@cosmo0920
Copy link
Member Author

Sure. Let's go ahead!

@ashie
Copy link
Member

ashie commented Mar 6, 2024

Done.

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