Skip to content
This repository was archived by the owner on Aug 29, 2023. It is now read-only.

Conversation

@dapplion
Copy link
Contributor

I believe abortable is not necessary in this package, since we can kill the socket directly.

socket.end()
},

source: (options.signal != null) ? abortableSource(source, options.signal) : source,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Socket source will abort when destroying it

async sink (source) {
if ((options?.signal) != null) {
source = abortableSource(source, options.signal)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any usecase to having to abort the remote socket read source during dial

@achingbrain achingbrain self-requested a review October 18, 2022 15:42
@mpetrunic mpetrunic added the need/maintainers-input Needs input from the current maintainer(s) label Nov 8, 2022
@achingbrain achingbrain changed the title Remove abortable fix: remove abortable Dec 12, 2022
@achingbrain achingbrain changed the title fix: remove abortable fix: remove abortable-iterator and close socket directly on abort Dec 12, 2022
Copy link
Contributor Author

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Thanks for completing! ❤️ LGTM

@achingbrain achingbrain merged commit 28fe750 into libp2p:master Dec 13, 2022
github-actions bot pushed a commit that referenced this pull request Dec 13, 2022
## [6.0.6](v6.0.5...v6.0.6) (2022-12-13)

### Bug Fixes

* remove abortable-iterator and close socket directly on abort ([#220](#220)) ([28fe750](28fe750))
@github-actions
Copy link

🎉 This PR is included in version 6.0.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dapplion dapplion deleted the dapplion/remove-abortable branch December 14, 2022 07:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

need/maintainers-input Needs input from the current maintainer(s) released

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants