You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/10/20 17:02:39 UTC
[GitHub] [trafficserver] sudheerv opened a new pull request #7285: Race condition between Server session release and Net read
sudheerv opened a new pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285
This fixes #7284
Defer Http1ServerSession destroying to prevent read buffer
corruption due to race with network i/o
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-868674834
@sudheerv If this is still an issue please reopen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-769453313
@sudheerv Can you please rebase this and @shinrich will take a look at it then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop removed a comment on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
zwoop removed a comment on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-715447821
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop removed a comment on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
zwoop removed a comment on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-715422754
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-713819918
What are the cases where the server_netvc is doing a read from a different thread? The server_netvc and the client_netvc and the HttpSM are all associated with the same ET_NET thread for the duration of the HttpSM. So all the net I/O should be performed in the same ET_NET thread. In that case the buffer frees in Http1ServerSession::do_io_close are serialized with the READ/WRITE event processing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-714575800
[approve ci FreeBSD]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-716668469
> What are the cases where the server_netvc is doing a read from a different thread? The server_netvc and the client_netvc and the HttpSM are all associated with the same ET_NET thread for the duration of the HttpSM. So all the net I/O should be performed in the same ET_NET thread. In that case the buffer frees in Http1ServerSession::do_io_close are serialized with the READ/WRITE event processing.
>
This seems to happen when we use a plugin which creates response Transform. We've a couple of custom plugins that perform Server Side Rendering (sort of like combo_handler plugin) by parsing a HTML origin response and fire off sideways calls to fetch the assets listed in the HTML response and then eventually stitch those responses together following a template (HTML in some cases, dust in some cases). The sideways requests are performed using TS Fetch API. I tried to set the thread affinity in the transform continuation and the sideways Fetch calls, yet couldn't seem to force all the associated requests to the same thread.
> In my reading of the current master, we may need to add do_io_read(nullptr, 0, nullptr) and do_io_write(nullptr, 0, nullptr) in the Http1ServerSession::do_io_close to ensure that a later net/io event does not get sent back to the now deleted continuation. But if the server_vc is still set, the closed flag should already be set which should block any future net/IO on that netvc.
The problem is do_io_close() is called from SM/Tunnel continuations which presumably guard using SM/Tunnel mutex. But, somehow that doesn't seem to block the race condition which makes me think that when Transforms are at play, the network read VIO isn't really being guarded by the SM/Tunnel Mutex anymore. Either way, it feels that once do_io_close() is called and it resets the continuation (or even a do_io_read(null, 0, null) for that matter) the network i/o is unblocked and potentially try to use the now freed MIOBuffer for the read. What we really need is a dedicated mutex for net read and use that mutex to lock the SM/Tunnel as well. But. I did not want to affect the performance for the normal cases as well as also introduce complexity by having to handle such lock failures in the SM/Tunnel continuations. So, as a compromise, I basically deferred/delayed the buffer release until the server session is detached from the SM and is put on the idle pool at which point adding an add
itional mutex should not impact request processing or latency. We can discuss more on slack/zoom or during the summit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv removed a comment on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
sudheerv removed a comment on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-714814921
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv removed a comment on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
sudheerv removed a comment on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-714575800
[approve ci FreeBSD]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-717355445
> I think it would be good to write an autest to exercise this scenario. I was able to add an autest in PR #7295 that exercised the threading problem that @calavera was in the SSN_START hook. I'll try to add one for this case. Is your plugin like the multiplexer? Except some part of the logic is executed from a Task thread?
That'd be awesome! Thanks @shinrich
The plugin is more like a esi/combo-handler. So, user requests a base page URL. The plugin creates a response transform on the HTML response, parses it, extracts the embedded links for static assets etc in the response and fetches those assets and replaces those links with the fetched responses. The final response going to the user is the fully stitched response with pretty much all the links resolved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-713819918
What are the cases where the server_netvc is doing a read from a different thread? The server_netvc and the client_netvc and the HttpSM are all associated with the same ET_NET thread for the duration of the HttpSM. So all the net I/O should be performed in the same ET_NET thread. In that case the buffer frees in Http1ServerSession::do_io_close are serialized with the READ/WRITE event processing.
In my reading of the current master, we may need to add do_io_read(nullptr, 0, nullptr) and do_io_write(nullptr, 0, nullptr) in the Http1ServerSession::do_io_close to ensure that a later net/io event does not get sent back to the now deleted continuation. But if the server_vc is still set, the closed flag should already be set which should block any future net/IO on that netvc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-715422754
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-717224136
I think it would be good to write an autest to exercise this scenario. I was able to add an autest in PR #7295 that exercised the threading problem that @calavera was in the SSN_START hook. I'll try to add one for this case. Is your plugin like the multiplexer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall closed pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
bryancall closed pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-715447821
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-714814921
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich edited a comment on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
shinrich edited a comment on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-717224136
I think it would be good to write an autest to exercise this scenario. I was able to add an autest in PR #7295 that exercised the threading problem that @calavera was in the SSN_START hook. I'll try to add one for this case. Is your plugin like the multiplexer? Except some part of the logic is executed from a Task thread?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] commented on pull request #7285: Race condition between Server session release and Net read
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7285:
URL: https://github.com/apache/trafficserver/pull/7285#issuecomment-864211363
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org