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

Implement progressive call results #69

Merged

Conversation

muzzammilshahid
Copy link
Member

No description provided.

@muzzammilshahid muzzammilshahid force-pushed the progressive-call-results branch 2 times, most recently from 864150b to 2c47c93 Compare September 5, 2024 13:40
@@ -302,7 +306,10 @@ func AsInt64(i interface{}) (int64, bool) {
case int32:
return int64(v), true
case uint:
return int64(v), true
if v > math.MaxInt64 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this check needed?

dealer.go Outdated
@@ -108,19 +113,28 @@ func (d *Dealer) ReceiveMessage(sessionID int64, msg messages.Message) (*Message
break
}

receiveProgress, ok := call.Options()[OptionReceiveProgress].(bool)
if !ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check can be removed and the value of receiveProgress maybe used directly without reassining.

Suggested change
if !ok {
if !ok {

dealer.go Outdated
@@ -131,13 +145,23 @@ func (d *Dealer) ReceiveMessage(sessionID int64, msg messages.Message) (*Message
return nil, fmt.Errorf("yield: not pending calls for session %d", sessionID)
}

delete(d.pendingCalls, yield.RequestID())
progress, ok := yield.Options()[OptionProgress].(bool)
if !ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, we can avoid the reassigning

session.go Outdated
@@ -60,7 +60,10 @@ func (w *Session) SendMessage(msg messages.Message) ([]byte, error) {
return data, nil
case messages.MessageTypeYield:
yield := msg.(*messages.Yield)
w.invocationRequests.Delete(yield.RequestID())
progress, ok := yield.Options()[OptionProgress].(bool)
if !ok || !progress {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !ok || !progress {
if !progress {

should also work

session.go Outdated
if !exists {
return nil, fmt.Errorf("received RESULT for invalid requestID")
}

progress, ok := result.Details()[OptionProgress].(bool)
if !ok || !progress {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !ok || !progress {
if !progress {

try

Copy link
Member

@om26er om26er left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good. But we need to add tests now. I am using wampproto-go in production in a product and landing changes without tests is worrying.

@muzzammilshahid muzzammilshahid merged commit fd8f83f into xconnio:main Sep 20, 2024
1 check passed
@muzzammilshahid muzzammilshahid deleted the progressive-call-results branch September 20, 2024 09:12
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.

2 participants