Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netty connector/container modifications #4387

Merged
merged 7 commits into from
Feb 19, 2020
Merged

Conversation

senivam
Copy link
Contributor

@senivam senivam commented Feb 5, 2020

POC adaptation for netty-connector (from #4286) + fix for netty-container (which depends on NettyInputStream).

Signed-off-by: Maxim Nesen maxim.nesen@oracle.com

Copy link

@olotenko olotenko left a comment

Choose a reason for hiding this comment

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

This looks very similar to the POC. I am happy with the changes.

What would make sense in a production version, is a bunch of limits on the pooled connections - idle timeouts, max total pool size, max pool size per destination. But that probably can be accomplished as a separate commit.

@senivam
Copy link
Contributor Author

senivam commented Feb 6, 2020

CQ #:21591

CQ Approved.

@jansupol
Copy link
Contributor

Please rebase on top of #4393

Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
Copy link

@olotenko olotenko left a comment

Choose a reason for hiding this comment

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

Because the implementation of available has been added, I have to change my original statement.

This is not an ok implementation of available. Please, update as specified in the comment to that commit.

Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
return peek.readableBytes();
}
return 0;
return buffer == null ? 0 : buffer.remaining();
Copy link
Contributor

Choose a reason for hiding this comment

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

should be synchronized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link

@olotenko olotenko Feb 18, 2020

Choose a reason for hiding this comment

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

Totally disagree. NettyInputStream has been, and remains, not thread safe. available, read and close methods have never been, and are not synchronized, therefore it has always been assumed to be used by a single thread, or access is synchronized by out of band means. Adding synchronized for available alone is pointless. Please, either remove it, or justify Jersey assumptions, and make all content input streams for all connectors thread safe.

For example, close is synchronized now, because it interacts with publish.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. We need to synchronize isList and reading. So only awaitNext(), publish(), and cleanup() are needed to be synchronized. We can remove it from available, close, releaseByteBuf and complete. Or do I miss something?

Copy link

@olotenko olotenko Feb 18, 2020

Choose a reason for hiding this comment

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

No, just do not add to available and releaseByteBuf.

publish, awaitNext and close interact with each other. A simple way to achieve total order of state transitions between things that are mostly done by two threads, and very rarely, is to use synchronized.

So we have one thread (Netty event loop) that receives byte buffers from the socket, and hands these buffers off to a different thread (Jersey thread that wants to readEntity). This is done through publish, and is potentially observed by awaitNext. publish also needs to see if maybe close has been called, so it does not assume anyone is going to release the buffers in the future. That's the purpose of synchronized around those three. It's possible to reduce synchronized block of close to just the place where end=true is set, but I think it will be fine even like this - mostly close will be called after all chunks have been passed through publish, and only maybe in some very rare cases someone will close the stream before reading the full Entity or whatever binary blob they are receiving.

synchronized on cleanup is not harmful, but not necessary, because it is invoked from synchronized sections. synchronized on available I think is low cost, but because it does not achieve any improved correctness, I would recommend not adding it. synchronized on releaseByteBuf can be seen as a little harmful - it will potentially add synchronization around current and buffer that are accessed only single-threadedly at that stage. (current is modified by the same thread that does read - publish only notifies awaitNext when new chunks arrive to isList)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like we agree here.

  • Either we have publish, awaitNext, complete and close synchronized
  • Or we have publish, awaitNext, and we move the synchronized from complete and close down to cleanup, where end=true and mainly if (reading) is used.
    Both are fine, though having close() synchronized could be a bit misleading as the NettyInputStream is not threadsafe.

Choose a reason for hiding this comment

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

I think you are right, moving synchronized to the methods that are not part of InputStream is better. I agree it will retain the intended logic. So, need to make sure publish, complete, awaitNext and cleanup synchronized, remove synchronized from the others to avoid confusion about thread safety in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, as per discussion, I've implemented the B) way - compete and close (including available and releaseByteBuf) are not synchronized, cleanup became synchronized.

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
Copy link

@olotenko olotenko left a comment

Choose a reason for hiding this comment

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

Remove synchronized on available. It serves no purpose. See comments.

Copy link
Contributor

@jansupol jansupol left a comment

Choose a reason for hiding this comment

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

Please remove synchronized as discussed

…nup synchronized

Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
return peek.readableBytes();
}
return 0;
return buffer == null ? 0 : buffer.remaining();

Choose a reason for hiding this comment

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

I think you are right, moving synchronized to the methods that are not part of InputStream is better. I agree it will retain the intended logic. So, need to make sure publish, complete, awaitNext and cleanup synchronized, remove synchronized from the others to avoid confusion about thread safety in the future.

return peek.readableBytes();
}
return 0;
return buffer == null ? 0 : buffer.remaining();

Choose a reason for hiding this comment

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

LGTM

@senivam senivam merged commit a19a446 into eclipse-ee4j:master Feb 19, 2020
@senivam senivam deleted the netty-ext branch February 19, 2020 10:36
@senivam senivam added this to the 2.30.1 milestone Feb 19, 2020
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