io: Clean up split check

[LucioFranco]

No description provided.

[udoprog]

Losing SplitStreamId means losing the ability to use it in an associative container as was part of the motivation in #1762. Is this OK?

CC: @de-vri-es since they are the one who wanted this.

[LucioFranco]

@udoprog good catch, Yeah, I'm curious if there is a better way to do it rather than that split. Carl was mentioning that there could be issues with using a usize cast from the ptr.

@de-vri-es would you mind explaining more of your use case? Just opened this PR because of a discussion with carl this morning but we could have totally have forgotten the full context :D

[carllerche]

If the goal is to use it in some associative container, the implementer of the container can generate unique identifiers on insertion or can define their own wrappers.

I am 👍 on adding a check via a fn (is_pair_of is fine w/ me). Given the dangling problem and the the fact that "split" is a utility type that can be implemented outside of tokio, I am inclined to remove the identifier type.

[carllerche]

Merging master should fix CI.

[de-vri-es]

To be more clear what I wanted to do with it: a server would maintain a map with mutexed write halfs. The server also spawns a task that uses the read half of each peer and looks up the write half if it needs to reply. Sometimes, it will use the map to broadcast a message to all peers.

An alternative to the map would be to create an Arc<Mutex<WriteHalf>> per peer and also stuff them in a vector for broadcasting. But thats basically an Arc<Mutex<Arc>>. The map solution seems more elegant.

Custom ID generation or a custom split type would also work, but I was mainly looking for roadblocks to making simple server applications. The pointer in the Arc is pretty nice as ID. If you use a counter for ID, you need to check if it isn't in use in the map already. As long as an ID is used as key for a map storing the write half, it can't dangle and the ID is guaranteed not to be in the map yet.

I'm curious though, why could the cast of pointer to usize be a problem here? It doesn't use unsafe code anywhere.

[m-ou-se]

Given the dangling problem and the the fact that "split" is a utility type that can be implemented outside of tokio

I don't think it can be done outside of Tokio without overhead. There would at least need to be something like .stream_ptr() -> *mut T to do it without storing extra information and changing/wrapping types.

[carllerche]

@m-ou-se I don't think I follow. The io::split fn could be copy / pasted outside of Tokio and modified to provide whatever additional capabilities. No additional overhead is required.

[m-ou-se]

Ah sorry, I misunderstood. I thought you meant to just move this 'stream id' functionality out of Tokio, not all of the split functionality.

[LucioFranco]

@de-vri-es I think the memory could be reused and thus you could get an id that is the same, since there is no generation attached to it you can't tell the difference. That is my guess.

Would it work to just (usize, WriteHalf) and look up based on that? I don't think this needs something specific to tokio.

I kinda agree with carl, maybe if this is something we want in tokio we could add a similar type in tokio-util that provides an id?

[de-vri-es]

@de-vri-es I think the memory could be reused and thus you could get an id that is the same, since there is no generation attached to it you can't tell the difference. That is my guess.

ID's can't be reused if they're used as key in a map storing the write half, since the Arc still exists at that time.

If you keep the IDs around separately, sure, the ID could be re-used. However, this isn't unsafe or anything. Worst case the following unsplit call panics.

Would it work to just (usize, WriteHalf) and look up based on that? I don't think this needs something specific to tokio.

Sure, you can manually generate IDs from a counter, but those are more error prone than the Arc pointer if used as a key in the map.

I kinda agree with carl, maybe if this is something we want in tokio we could add a similar type in tokio-util that provides an id?

It seems overkill to me to duplicate the whole split functionality for such a small change.

[m-ou-se]

I think the memory could be reused and thus you could get an id that is the same, since there is no generation attached to it you can't tell the difference. That is my guess.

Yes, but as long as one of the halves still exists, the ID still uniquely refers to the stream. The ID is only used to find the other half in a for example a HashMap, given the other one. If neither of the halves exist anymore, it doesn't matter that the ID might be re-used, as there is nothing to unsplit anyway. (You can't even call x.unsplit(y) if there are no x or y anymore, of course.)

[carllerche]

I am going to merge this as is now to unblock releasing. We can continue design discussion on an issue (feel free to open one and link it here).

[LucioFranco]

@de-vri-es @m-ou-se sorry for the delay on all this, would either of you be willing to open an issue about what we are discussing here and with maybe a concrete example I could peek at? Im curious :)