Skip to content

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jan 3, 2019

@MrMage not sure if this is the desired ordering? @Lukasa wasn't sure about the specifics.

@MrMage
Copy link
Collaborator

MrMage commented Jan 3, 2019

Hmm; tough question. To be honest, I don't quite remember.

I think we'd want to instantly add the call handler, to avoid accidentally dropping messages sent in between removing self and adding the call handler. Given that this change only adds the call handler after removing self, would that still be guaranteed after merging this change? /ccing @Lukasa

Also, I'm not sure how this would interact with #350. Would you mind if I waited with merging this until that PR has been merged?

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 3, 2019

I think we'd want to instantly add the call handler, to avoid accidentally dropping messages sent in between removing self and adding the call handler.

No such timing window should exist.

The callback (when attached to a promise) from removing self will fire when self is no longer in the pipeline, but while self's ChannelHandlerContext is still valid. Given that removing self always runs synchronously when run from on the event loop, we should be just fine.

With that said, currently there is no good interface that actually promises this behaviour. We should probably consider adding some explicitly thread-unsafe interfaces to ChannelHandlerContext that promise that you can actually remove self synchronously (and add other handlers to ctx.pipeline synchronously) such that we don't encounter this kind of speculation.

@glbrntt
Copy link
Collaborator Author

glbrntt commented Jan 3, 2019

Also, I'm not sure how this would interact with #350. Would you mind if I waited with merging this until that PR has been merged?

No problem!

@MrMage
Copy link
Collaborator

MrMage commented Jan 3, 2019

I think we'd want to instantly add the call handler, to avoid accidentally dropping messages sent in between removing self and adding the call handler.

No such timing window should exist.

The callback (when attached to a promise) from removing self will fire when self is no longer in the pipeline, but while self's ChannelHandlerContext is still valid. Given that removing self always runs synchronously when run from on the event loop, we should be just fine.

With that said, currently there is no good interface that actually promises this behaviour. We should probably consider adding some explicitly thread-unsafe interfaces to ChannelHandlerContext that promise that you can actually remove self synchronously (and add other handlers to ctx.pipeline synchronously) such that we don't encounter this kind of speculation.

Sounds good enough to me, thanks for the clarification!

@glbrntt I'm happy with the PR; will merge one #350 has been merged (which might require changes in this PR). Just ping me when that's done :-)

@sergiocampama
Copy link
Contributor

Actually, #350 still needs to wait until apple/swift-nio-http2#36 is merged and released. You should go ahead and merge this one and I'll rebase it onto my PR.

@MrMage MrMage merged commit a43337c into grpc:master Jan 6, 2019
@glbrntt glbrntt deleted the swift_nio_1_12 branch April 12, 2019 13:18
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.

4 participants