Skip to content

feat: features usage of the exact negotiated stream id limit#58

Merged
wmnsk merged 9 commits intowmnsk:mainfrom
gomaja:feature-negotiated-stream-id-usage
Jun 18, 2025
Merged

feat: features usage of the exact negotiated stream id limit#58
wmnsk merged 9 commits intowmnsk:mainfrom
gomaja:feature-negotiated-stream-id-usage

Conversation

@gomaja
Copy link
Copy Markdown
Contributor

@gomaja gomaja commented May 20, 2025

Hi @wmnsk

This PR updates to the latest Ishida Wataru SCTP dependency library. With this update, we feature getting the maximum negotiated stream ID, which can be used by the user. Exceeding the max negotiated stream ID will throw an error from the system.

Copy link
Copy Markdown
Owner

@wmnsk wmnsk left a comment

Choose a reason for hiding this comment

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

Thanks! Two things:

  • as a library used behind apps, I wouldn’t like to log unless users explicitly enable it. I think they can just be removed, but if you think they’re really important, consider adding logger like this. IMO just logging without returning error is not really useful though.
  • Please don’t leave comments here and there - if they’re important for users, please add them in godoc. Otherwise, remove them (or, if the code may confuse developers, leave some explanations but not every line).

Comment thread conn.go
Comment thread conn.go Outdated
@gomaja
Copy link
Copy Markdown
Contributor Author

gomaja commented May 21, 2025

Thanks for the review.
I noticed that you already have the logger, so I used it.

@gomaja gomaja requested a review from wmnsk May 21, 2025 19:06
@wmnsk
Copy link
Copy Markdown
Owner

wmnsk commented May 30, 2025

sorry I’ve been on vacation and not in front of my computer. I think I can review next week.

Copy link
Copy Markdown
Owner

@wmnsk wmnsk left a comment

Choose a reason for hiding this comment

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

Left a lot of comments, but mostly the same stuff. Thanks for your patience!

Comment thread aspsm.go Outdated
Comment thread aspsm.go Outdated
Comment thread client.go Outdated
Comment thread client.go Outdated
Comment thread client.go Outdated
Comment thread fsm.go Outdated
Comment thread server.go Outdated
Comment thread server.go Outdated
Comment thread server.go Outdated
Comment thread server.go Outdated
gomaja and others added 4 commits June 10, 2025 20:09
Co-authored-by: Yoshiyuki Kurauchi <ahochauwaaaaa@gmail.com>
Co-authored-by: Yoshiyuki Kurauchi <ahochauwaaaaa@gmail.com>
@gomaja
Copy link
Copy Markdown
Contributor Author

gomaja commented Jun 11, 2025

Thanks for the review.
Necessary changes are treated, and some other changes that require deeper investigation will be made on a separate PR.

@gomaja gomaja requested a review from wmnsk June 11, 2025 13:22
Comment thread client.go Outdated
Comment thread client.go Outdated
Comment thread client.go Outdated
Comment thread client.go Outdated
Comment thread conn.go Outdated
Comment thread conn.go
Comment thread server.go
@gomaja
Copy link
Copy Markdown
Contributor Author

gomaja commented Jun 16, 2025

Thanks for the review.
Necessary changes are treated and seems better now.

@gomaja gomaja requested a review from wmnsk June 16, 2025 11:00
Copy link
Copy Markdown
Owner

@wmnsk wmnsk left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for your contribution!
Perhaps we could look into another way to cleanup the underlying connection in case of errors, but not necessary here :)

@wmnsk wmnsk merged commit fb734d3 into wmnsk:main Jun 18, 2025
3 checks passed
@gomaja gomaja deleted the feature-negotiated-stream-id-usage branch December 27, 2025 21:27
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