events: Ignore send context #23500
Merged
events: Ignore send context #23500
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When sending an event asynchornously, the original context used for whatever generated the event (probably a synchronous, quick HTTP context) is probably not what is wanted for sending the event, which could face delays if a consumer is backed up. I will admit myself to sometimes having "context blindness", where I just take whatever context is incoming in a function and thread it out to all calls. Normally this is the right thing to do when, say, tying downstream API calls to an upstream HTTP timeout. When making KV events, for example, we used the HTTP context for `SendEvent()`, and this can cause the events to be dropped if they aren't taken from the channel before the HTTP request finishes. In retrospect, it was probably unnecessary to include a context in the `SendEvent` interface. We keep the context in place for backwards compability, but also in case we want to use it for purposes other than timeouts and cancellations in the future.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
github-actions
bot
added
the
hashicorp-contributed-pr
|
Build Results: |
|
CI Results: |
benashz
approved these changes
Oct 5, 2023
vault/eventbus/bus.go
Outdated
| @@ -114,7 +114,8 @@ func patchMountPath(data *logical.EventData, pluginInfo *logical.EventPluginInfo | |||
| // This function is meant to be used by trusted internal code, so it can specify details like the namespace | |||
| // and plugin info. Events from plugins should be routed through WithPlugin(), which will populate | |||
| // the namespace and plugin info automatically. | |||
| func (bus *EventBus) SendEventInternal(ctx context.Context, ns *namespace.Namespace, pluginInfo *logical.EventPluginInfo, eventType logical.EventType, data *logical.EventData) error { | |||
| // The context passed in is currently ignored. | |||
There was a problem hiding this comment.
nit: although the reason for ignoring the context is well documented in the PR, you may want to include a brief summary here as well.
|
Thanks! |
swenson
pushed a commit
that referenced
this pull request
Oct 5, 2023
When sending an event asynchronously, the original context used for whatever generated the event (probably a synchronous, quick HTTP context) is probably not what is wanted for sending the event, which could face delays if a consumer is backed up. I will admit myself to sometimes having "context blindness", where I just take whatever context is incoming in a function and thread it out to all calls. Normally this is the right thing to do when, say, tying downstream API calls to an upstream HTTP timeout. When making KV events, for example, we used the HTTP context for `SendEvent()`, and this can cause the events to be dropped if they aren't taken from the channel before the HTTP request finishes. In retrospect, it was probably unnecessary to include a context in the `SendEvent` interface. We keep the context in place for backwards compability, but also in case we want to use it for purposes other than timeouts and cancellations in the future.
swenson
pushed a commit
that referenced
this pull request
Oct 5, 2023
When sending an event asynchronously, the original context used for whatever generated the event (probably a synchronous, quick HTTP context) is probably not what is wanted for sending the event, which could face delays if a consumer is backed up. I will admit myself to sometimes having "context blindness", where I just take whatever context is incoming in a function and thread it out to all calls. Normally this is the right thing to do when, say, tying downstream API calls to an upstream HTTP timeout. When making KV events, for example, we used the HTTP context for `SendEvent()`, and this can cause the events to be dropped if they aren't taken from the channel before the HTTP request finishes. In retrospect, it was probably unnecessary to include a context in the `SendEvent` interface. We keep the context in place for backwards compability, but also in case we want to use it for purposes other than timeouts and cancellations in the future.
swenson
pushed a commit
that referenced
this pull request
Oct 5, 2023
When sending an event asynchronously, the original context used for whatever generated the event (probably a synchronous, quick HTTP context) is probably not what is wanted for sending the event, which could face delays if a consumer is backed up. I will admit myself to sometimes having "context blindness", where I just take whatever context is incoming in a function and thread it out to all calls. Normally this is the right thing to do when, say, tying downstream API calls to an upstream HTTP timeout. When making KV events, for example, we used the HTTP context for `SendEvent()`, and this can cause the events to be dropped if they aren't taken from the channel before the HTTP request finishes. In retrospect, it was probably unnecessary to include a context in the `SendEvent` interface. We keep the context in place for backwards compability, but also in case we want to use it for purposes other than timeouts and cancellations in the future. Co-authored-by: Christopher Swenson <christopher.swenson@hashicorp.com>
swenson
pushed a commit
that referenced
this pull request
Oct 5, 2023
When sending an event asynchronously, the original context used for whatever generated the event (probably a synchronous, quick HTTP context) is probably not what is wanted for sending the event, which could face delays if a consumer is backed up. I will admit myself to sometimes having "context blindness", where I just take whatever context is incoming in a function and thread it out to all calls. Normally this is the right thing to do when, say, tying downstream API calls to an upstream HTTP timeout. When making KV events, for example, we used the HTTP context for `SendEvent()`, and this can cause the events to be dropped if they aren't taken from the channel before the HTTP request finishes. In retrospect, it was probably unnecessary to include a context in the `SendEvent` interface. We keep the context in place for backwards compability, but also in case we want to use it for purposes other than timeouts and cancellations in the future. Co-authored-by: Christopher Swenson <christopher.swenson@hashicorp.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When sending an event asynchornously, the original context used for whatever generated the event (probably a synchronous, quick HTTP context) is probably not what is wanted for sending the event, which could face delays if a consumer is backed up.
I will admit myself to sometimes having "context blindness", where I just take whatever context is incoming in a function and thread it out to all calls. Normally this is the right thing to do when, say, tying downstream API calls to an upstream HTTP timeout.
When making KV events, for example, we used the HTTP context for
SendEvent(), and this can cause the events to be dropped if they aren't taken from the channel before the HTTP request finishes.In retrospect, it was probably unnecessary to include a context in the
SendEventinterface.We keep the context in place for backwards compability, but also in case we want to use it for purposes other than timeouts and cancellations in the future.