On the surface, the "fix" might be as simple as checking for sub.peers.size rather than sub.peers.values().length (because Map iterators have no such property by default).
Looking deeper, that change would exercise code which was likely never executing prior, and may have the following side effects compared to the local Peer.publish() fallback (which was presumably always safe):
If any of these seem like genuine issues, I'd be happy to submit a PR, but I'd want some guidance on how to appropriately test proposed changes. Do you have a solution for mocking a network of peers?
What OS are you using (
uname -a
, or Windows version)?Darwin Kernel Version 22.4.0: Mon Mar 6 21:01:02 PST 2023; root:xnu-8796.101.5~3/RELEASE_ARM64_T8112 arm64 Mac OS Ventura 13.3.1 (22E261)
What version Socket Runtime are you using?
Reading an unreleased commit after v0.5.4 -- https://github.com/socketsupply/socket/tree/0c235fb4fa97f552c759551c9daab815a96eeaa9
What programming language are you using (C/C++/Go/Rust)?
N/A (but usually Javascript/Typescript)
What did you expect to see and what you saw instead?
When investigating how/if socket apps can disconnect from the p2p-network (looks like the answer might be
(await network({})).peer.close()
as opposed to(await network({})).disconnect()
) I spotted what appears to be a network optimization that is never executed when invokingsubcluster.emit()
. https://github.com/socketsupply/socket/blob/0c235fb4fa97f552c759551c9daab815a96eeaa9/api/stream-relay/sugar.js#L256On the surface, the "fix" might be as simple as checking for
sub.peers.size
rather thansub.peers.values().length
(becauseMap
iterators have no such property by default).Looking deeper, that change would exercise code which was likely never executing prior, and may have the following side effects compared to the local
Peer.publish()
fallback (which was presumably always safe):"#packet"
events are emitted to thesocket
/bus
as otherwise would occur during aPeer.publish()
https://github.com/socketsupply/socket/blob/0c235fb4fa97f552c759551c9daab815a96eeaa9/api/stream-relay/index.js#L974-L984 https://github.com/socketsupply/socket/blob/0c235fb4fa97f552c759551c9daab815a96eeaa9/api/stream-relay/sugar.js#L264-L272 https://github.com/socketsupply/socket/blob/0c235fb4fa97f552c759551c9daab815a96eeaa9/api/stream-relay/sugar.js#L60-L63_peer.mcast()
will haveusr4
andusr3
set to thepeerId
of the local peer and the first remote peer respectively -- and will be of typePacketStream
rather thanPacketPublish
https://github.com/socketsupply/socket/blob/0c235fb4fa97f552c759551c9daab815a96eeaa9/api/stream-relay/sugar.js#L260-L261 https://github.com/socketsupply/socket/blob/0c235fb4fa97f552c759551c9daab815a96eeaa9/api/stream-relay/index.js#L200-L204 https://github.com/socketsupply/socket/blob/0c235fb4fa97f552c759551c9daab815a96eeaa9/api/stream-relay/index.js#L972RemotePeer.write()
was getting exercised anywhere, and though I doubt it's the case, it might not work as expected -- one place it's potentially getting exercised is below, which seems to use a possibly stale reference to the closure'speer.write
instead ofee._peer.write
https://github.com/socketsupply/socket/blob/0c235fb4fa97f552c759551c9daab815a96eeaa9/api/stream-relay/sugar.js#L317-L325If any of these seem like genuine issues, I'd be happy to submit a PR, but I'd want some guidance on how to appropriately test proposed changes. Do you have a solution for mocking a network of peers?