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

Documentation needed (PublishWithContext does not use context) #195

Closed
Av1shay opened this issue May 19, 2023 · 7 comments · Fixed by #259
Closed

Documentation needed (PublishWithContext does not use context) #195

Av1shay opened this issue May 19, 2023 · 7 comments · Fixed by #259
Assignees
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Milestone

Comments

@Av1shay
Copy link

Av1shay commented May 19, 2023

Describe the bug

I'm not sure if I'm blind or something, but looking at the function PublishWithContext I can't see where it uses the context? it calls ch.PublishWithDeferredConfirmWithContext and inside it just check if ctx == nil, after that it does not use the ctx. What am I missing? (Version 1.8.1)

func (ch *Channel) PublishWithDeferredConfirmWithContext(ctx context.Context, exchange, key string, mandatory, immediate bool, msg Publishing) (*DeferredConfirmation, error) {
	if ctx == nil {
		return nil, errors.New("amqp091-go: nil Context")
	}

	if err := msg.Headers.Validate(); err != nil {
		return nil, err
	}

	ch.m.Lock()
	defer ch.m.Unlock()

	var dc *DeferredConfirmation
	if ch.confirming {
		dc = ch.confirms.publish()
	}

	if err := ch.send(&basicPublish{
		Exchange:   exchange,
		RoutingKey: key,
		Mandatory:  mandatory,
		Immediate:  immediate,
		Body:       msg.Body,
		Properties: properties{
			Headers:         msg.Headers,
			ContentType:     msg.ContentType,
			ContentEncoding: msg.ContentEncoding,
			DeliveryMode:    msg.DeliveryMode,
			Priority:        msg.Priority,
			CorrelationId:   msg.CorrelationId,
			ReplyTo:         msg.ReplyTo,
			Expiration:      msg.Expiration,
			MessageId:       msg.MessageId,
			Timestamp:       msg.Timestamp,
			Type:            msg.Type,
			UserId:          msg.UserId,
			AppId:           msg.AppId,
		},
	}); err != nil {
		if ch.confirming {
			ch.confirms.unpublish()
		}
		return nil, err
	}

	return dc, nil
}

Reproduction steps

none

Expected behavior

Honor context cancellation signals

Additional context

No response

@Av1shay Av1shay added the bug Something isn't working label May 19, 2023
@Av1shay Av1shay changed the title why PublishWithContext does not use context Seems that PublishWithContext does not use context May 19, 2023
@lukebakken
Copy link
Contributor

lukebakken commented May 19, 2023

Hello, thanks for using this library and RabbitMQ. You can see in #96 where the contexts were added, and #140 for where their use was removed. If you think that was the wrong decision, please start a discussion here.

In the future, use git blame or "blame" feature of the GitHub code navigator to figure out for yourself code changes.

@lukebakken lukebakken closed this as not planned Won't fix, can't repro, duplicate, stale May 19, 2023
@nemith
Copy link

nemith commented Mar 21, 2024

In the future, use git blame or "blame" feature of the GitHub code navigator to figure out for yourself code changes.

This should be documented and not require people to code spelunking.

@lukebakken
Copy link
Contributor

@nemith please take the time to add the relevant documentation via a PR. We would appreciate it.

@lukebakken lukebakken reopened this Mar 22, 2024
@lukebakken
Copy link
Contributor

Re-opening because the current API has confused users. We can only change it in 2.0, however, so documentation is necessary.

@lukebakken lukebakken self-assigned this Mar 22, 2024
@lukebakken lukebakken added documentation Improvements or additions to documentation good first issue Good for newcomers and removed bug Something isn't working labels Mar 22, 2024
@lukebakken lukebakken added this to the 1.9.1 milestone Mar 22, 2024
@lukebakken lukebakken changed the title Seems that PublishWithContext does not use context Documentation needed (PublishWithContext does not use context) Mar 22, 2024
@laurentb-roy
Copy link

laurentb-roy commented Apr 17, 2024

Regarding this issue, I think it is fixable without a 2.0, if we do the following

  1. Remove the deprecation notice on the Publish() and PublishWithDeferredConfirm(). Their behavior right now are exactly the same as their WithContext variant.
  2. Remove the null check on the context in the WithContext publish function so that we can send a unused nil value
  3. Carefully document that the WithContext have the exact same behavior as the base function and that the context is not used (what is already planned for the 1.9.1)
  4. Update the examples to remove the usage of WithContext function so that beginner are not confused.

Since the right function already exists, we may have "get out of jail free card" for this.

I may try for a PR if the idea seems good. Thoughts?

@Zerpet
Copy link
Contributor

Zerpet commented Apr 18, 2024

@laurentb-roy I like your proposal. We intended to add context to all channel function in #124, however, we encountered other challenges along the way, see #124 (comment) and #124 (comment)

Happy to review a contribution with the proposed actions.

@Zerpet Zerpet self-assigned this May 6, 2024
Zerpet added a commit that referenced this issue May 6, 2024
The context was not honoured in any of the *WithContext functions. This
is confusing, and arguably broken. However, we cannot immediately fix
the context-support situation due to #124 (comment)

This commit undeprecates the non-context variants of publish, and
documents that both variants are equivalent. The example now favours the
non-context variants.

Related to #195

Signed-off-by: Aitor Perez Cedres <aitor.perez@broadcom.com>
Zerpet added a commit that referenced this issue May 6, 2024
The context was not honoured in any of the *WithContext functions. This
is confusing, and arguably broken. However, we cannot immediately fix
the context-support situation due to #124 (comment)

This commit undeprecates the non-context variants of publish, and
documents that both variants are equivalent. The example now favours the
non-context variants.

Related to #195

Signed-off-by: Aitor Perez Cedres <aitor.perez@broadcom.com>
@Zerpet
Copy link
Contributor

Zerpet commented May 6, 2024

I've opened a PR to address this issue, based on the feedback from @laurentb-roy. Anyone is welcome to provide any comments in the PR #259

Zerpet added a commit that referenced this issue May 7, 2024
The context was not honoured in any of the *WithContext functions. This
is confusing, and arguably broken. However, we cannot immediately fix
the context-support situation due to #124 (comment)

This commit undeprecates the non-context variants of publish, and
documents that both variants are equivalent. The example now favours the
non-context variants.

Related to #195

Signed-off-by: Aitor Perez Cedres <aitor.perez@broadcom.com>
lukebakken pushed a commit that referenced this issue May 7, 2024
The context was not honoured in any of the *WithContext functions. This
is confusing, and arguably broken. However, we cannot immediately fix
the context-support situation due to #124 (comment)

This commit undeprecates the non-context variants of publish, and
documents that both variants are equivalent. The example now favours the
non-context variants.

Related to #195

Signed-off-by: Aitor Perez Cedres <aitor.perez@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants