fix: Support for mapping multiple listener address #1183
Conversation
| @@ -287,7 +287,7 @@ proc new*( | |||
| interval = config.blockMaintenanceInterval, | |||
| numberOfBlocksPerInterval = config.blockMaintenanceNumberOfBlocks, | |||
| ) | |||
|
|
|||
| natManager = NatManager.new(config.nat) | |||
There was a problem hiding this comment.
🟡 Hm... not sure I'm very comfortable scoping the object that gets its reference passed onto another thread here. What you had before were a bunch of module-private globals so I think that was OK, but here you may find yourself with unwanted accesses to fields in this object if someone later doesn't realize this is not safe.
There was a problem hiding this comment.
Previously, we were using primitive globals, but now we need to store a sequence of entries—which isn't GC-safe—so we can't use globals anymore. I believe the current implementation is the simplest and cleanest approach. Do you have any alternative suggestions?
Note: Only a single thread accesses the NatManager attributes, so there's no concern around concurrent access.
There was a problem hiding this comment.
Here you're also initializing a ref object that contains a sequence and then passing it to another thread to access. What am I missing? How's that more GC-safe than having it as a module-private global? You could even have NatManager itself as a module-level private.
There was a problem hiding this comment.
Btw I realize we're not currently accessing NatManager from multiple threads, what I meant to say is that we are leaving a reference to it here which someone in the future might misuse.
| portMappings: seq[PortMapping] | ||
| thread: Thread[ptr NatManager] | ||
| config: NatConfig | ||
| upnp: Miniupnp |
There was a problem hiding this comment.
🔴 Those were threadvars before and initialized per-thread. Why are we not doing that anymore, and is it safe?
There was a problem hiding this comment.
@gmega I moved pnp and upnp into the NatManager object for sanity and consistency. They are initialized once by the main Chronos thread and then used by another thread. Since we retain a reference to NatManager throughout the lifecycle, I don’t foresee any issues. That said, we can still convert them to threadvars if you think it's more appropriate
There was a problem hiding this comment.
I wouldn't know, I don't know why they were done as threadvars in the first place, but it would seem to indicate that you can't initialize them in one thread then use them in another. Previously we had them as module-level globals, which are also stable references? Am I missing anything?
This PR will fix #1158