You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2022/02/23 21:47:54 UTC
[GitHub] [couchdb] kocolosk opened a new pull request #3940: Ensure the multipart parser always learns about the workers
kocolosk opened a new pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940
## Overview
This PR adds an extra `hello_from_writer` message into the handshake between the process that reads the multipart attachment data from the socket and the writer processes (potentially on remote nodes) that persist the data into each shard file. This ensures that even in the case where a writer does not end up asking for the data (e.g. because the revision already exists in the tree), the parser will monitor the writer and therefore know when the writer has exited.
The patch makes some assumptions that the attachment flush function is executed in the same process as the initial one that is spawned to handle the fabric_rpc work request. That's true today, but if it changed in the future it would be a non-obvious breakage to debug.
I'm not crazy about this solution to be quite honest, but I figured I could put it up for review anyway and see what others think.
## Testing recommendations
I added an extra test to the `elixir-suite` target. Without this patch you should find that the test eventually causes some other part of the suite to hang, because the Erlang process handling one of the open TCP connections is stuck waiting for the attachment parser to exit. Again, I wish the test failed in a more obvious way, but I guess it's better than nothing.
## Related Issues or Pull Requests
Fixes #3939
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814304096
##########
File path: src/fabric/src/fabric_rpc.erl
##########
@@ -638,16 +638,14 @@ make_att_readers([#doc{atts = Atts0} = Doc | Rest]) ->
[Doc#doc{atts = Atts} | make_att_readers(Rest)].
make_att_reader({follows, Parser, Ref}) ->
+ % This code will fail if the returned closure is called by a
+ % process other than the one that called make_att_reader/1 in the
+ % first place. The reason we don't put everything inside the
+ % closure is that the `hello_from_writer` message must *always* be
+ % sent to the parser, even if the closure never gets called.
+ ParserRef = erlang:monitor(process, Parser),
+ Parser ! {hello_from_writer, Ref, self()},
Review comment:
I had noticed we previously cached the PRef in the `mp_parser_ref` process dict. So, for multiple attachments we would have created only a single monitor.
In the PR we have a separate monitor for each attachment and send a `hello_from_writer` for each one. If it's just saving an extra few monitor references it's not a big deal (though they are remote, not sure actually how impactful those are...). But would multiple `hello_from_writer` affect the by fetching protocol correctness. Each one would end up overwriting the `WriterPid => {WriterRef, 0}` entry but monitors would stack up, then we could get multiple `DOWN` messages for each attachment instead of once per writer termination.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814268603
##########
File path: src/fabric/src/fabric_rpc.erl
##########
@@ -638,16 +638,14 @@ make_att_readers([#doc{atts = Atts0} = Doc | Rest]) ->
[Doc#doc{atts = Atts} | make_att_readers(Rest)].
make_att_reader({follows, Parser, Ref}) ->
+ % This code will fail if the returned closure is called by a
+ % process other than the one that called make_att_reader/1 in the
+ % first place. The reason we don't put everything inside the
+ % closure is that the `hello_from_writer` message must *always* be
+ % sent to the parser, even if the closure never gets called.
Review comment:
Wonder if we should assert that check explicitly -- remember the closure creator pid and in the closure double-check that `self() == FollowsClosureOwner`? Or would that be overkill.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814307503
##########
File path: src/couch/src/couch_httpd_multipart.erl
##########
@@ -104,6 +104,10 @@ mp_parse_atts(eof, {Ref, Chunks, Offset, Counters, Waiting}) ->
receive
abort_parsing ->
ok;
+ {hello_from_writer, Ref, WriterPid} ->
+ WriterRef = erlang:monitor(process, WriterPid),
+ NewCounters = orddict:store(WriterPid, {WriterRef, 0}, Counters),
+ mp_parse_atts(eof, {Ref, Chunks, Offset, NewCounters, Waiting});
Review comment:
It's only 2 lines but noticed it repeats 3 times, wonder if it would look simpler with a tiny helper function like `{WRef, NewCounters} = writer_init(WriterPid, Counters)`. But then again, it's smaller enough so it doesn't look a lot better, maybe not worth bothering.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814330404
##########
File path: src/couch/src/couch_httpd_multipart.erl
##########
@@ -104,6 +104,10 @@ mp_parse_atts(eof, {Ref, Chunks, Offset, Counters, Waiting}) ->
receive
abort_parsing ->
ok;
+ {hello_from_writer, Ref, WriterPid} ->
+ WriterRef = erlang:monitor(process, WriterPid),
+ NewCounters = orddict:store(WriterPid, {WriterRef, 0}, Counters),
+ mp_parse_atts(eof, {Ref, Chunks, Offset, NewCounters, Waiting});
Review comment:
I was on the fence but if we're adding extra logic to do at-most-once monitoring then a function probably does make sense (and could be refactored to share more in common with update_writer)
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814329743
##########
File path: src/fabric/src/fabric_rpc.erl
##########
@@ -638,16 +638,14 @@ make_att_readers([#doc{atts = Atts0} = Doc | Rest]) ->
[Doc#doc{atts = Atts} | make_att_readers(Rest)].
make_att_reader({follows, Parser, Ref}) ->
+ % This code will fail if the returned closure is called by a
+ % process other than the one that called make_att_reader/1 in the
+ % first place. The reason we don't put everything inside the
+ % closure is that the `hello_from_writer` message must *always* be
+ % sent to the parser, even if the closure never gets called.
+ ParserRef = erlang:monitor(process, Parser),
+ Parser ! {hello_from_writer, Ref, self()},
Review comment:
Ah, I totally missed the 1 parser + multiple attachments line of thinking. I was looking at the caching and figuring it was only there because the function was being invoked for every `get_bytes` call, but yes, what you're proposing definitely makes sense given the lifecycle of the parser. Thanks for pointing it out!
And yes, it makes sense to extend the test suite here.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814356307
##########
File path: src/couch/src/couch_httpd_multipart.erl
##########
@@ -134,6 +138,10 @@ mp_abort_parse_atts(_, _) ->
maybe_send_data({Ref, Chunks, Offset, Counters, Waiting}) ->
receive
+ {hello_from_writer, Ref, WriterPid} ->
+ WriterRef = erlang:monitor(process, WriterPid),
+ NewCounters = orddict:store(WriterPid, {WriterRef, 0}, Counters),
Review comment:
Yeah, agree. It seems we'd want to initialize each separate follows parser only once. Then I think it could work?
So, for example, we could fold over all docs [1] and all the attachments and keep a map of `#{ParserPid => Ref}`. If the `ParserPid` is not in the map, we send the `{hello_from_writer, Ref, WriterPid}` and start monitoring it. Then we use its `Ref` in the `receive` `DOWN'` inside the closure. If, `ParserPid` is in the map ,then we skip sending the `hello` message and just use its existing `Ref` in the `receive DOWN'`.
[1] It's seems it's not just a doc with multiple attachment but it could also be multiple docs, each with multiple attachments?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
kocolosk commented on pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#issuecomment-1057615647
@nickva @jcoglan I'm not sure I'll be able to return to this one in the short term given work commitments and some upcoming PTO. I have no issue with someone else picking this up, or mitigating the bug with a completely different tactic.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814356307
##########
File path: src/couch/src/couch_httpd_multipart.erl
##########
@@ -134,6 +138,10 @@ mp_abort_parse_atts(_, _) ->
maybe_send_data({Ref, Chunks, Offset, Counters, Waiting}) ->
receive
+ {hello_from_writer, Ref, WriterPid} ->
+ WriterRef = erlang:monitor(process, WriterPid),
+ NewCounters = orddict:store(WriterPid, {WriterRef, 0}, Counters),
Review comment:
Yeah, agree. It seems we'd want to initialize each separate follows parser only once. Then I think it could work?
So, for example, we could fold over all docs [1] and all the attachments and keep a map of `#{ParserPid => Ref}` in the accumulator (slightly nicer than a process dict!).
- If the `ParserPid` is not in the map:
* Start monitoring it
* Send the `{hello_from_writer, Ref, WriterPid}`
* Add it to the map
* Use its `Ref` in the `receive` `DOWN'` inside the closure
- If, `ParserPid` is in the map:
* Skip sending the `hello` message
* And just use its existing `Ref` value in the `receive DOWN'` in the closure.
[1] It's seems it's not just a doc with multiple attachment but it could also be multiple docs, each with multiple attachments?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814304096
##########
File path: src/fabric/src/fabric_rpc.erl
##########
@@ -638,16 +638,14 @@ make_att_readers([#doc{atts = Atts0} = Doc | Rest]) ->
[Doc#doc{atts = Atts} | make_att_readers(Rest)].
make_att_reader({follows, Parser, Ref}) ->
+ % This code will fail if the returned closure is called by a
+ % process other than the one that called make_att_reader/1 in the
+ % first place. The reason we don't put everything inside the
+ % closure is that the `hello_from_writer` message must *always* be
+ % sent to the parser, even if the closure never gets called.
+ ParserRef = erlang:monitor(process, Parser),
+ Parser ! {hello_from_writer, Ref, self()},
Review comment:
I had noticed we previously cached the PRef in the `mp_parser_ref` process dict. So, for multiple attachments we would have created only a single monitor.
In the PR we have a separate monitor for each attachment and send a `hello_from_writer` for each one. If it's just saving an extra few monitor references it's not a big deal (though they are remote, not sure actually how impactful those are...). But would multiple `hello_from_writer` affect the by fetching protocol correctness. Each one would end up overwriting the `WriterPid => {WriterRef, 0}` entry but monitors would stack up, then we could get multiple `DOWN` messages for each attachment instead of once per writer termination.
EDIT: Would it possible to extend the tests to have another (separate one) with multiple attachments, perhaps.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814291185
##########
File path: src/fabric/src/fabric_rpc.erl
##########
@@ -638,16 +638,14 @@ make_att_readers([#doc{atts = Atts0} = Doc | Rest]) ->
[Doc#doc{atts = Atts} | make_att_readers(Rest)].
make_att_reader({follows, Parser, Ref}) ->
+ % This code will fail if the returned closure is called by a
+ % process other than the one that called make_att_reader/1 in the
+ % first place. The reason we don't put everything inside the
+ % closure is that the `hello_from_writer` message must *always* be
+ % sent to the parser, even if the closure never gets called.
Review comment:
I like that idea.
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on a change in pull request #3940: Ensure the multipart parser always learns about the workers
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3940:
URL: https://github.com/apache/couchdb/pull/3940#discussion_r814341172
##########
File path: src/couch/src/couch_httpd_multipart.erl
##########
@@ -134,6 +138,10 @@ mp_abort_parse_atts(_, _) ->
maybe_send_data({Ref, Chunks, Offset, Counters, Waiting}) ->
receive
+ {hello_from_writer, Ref, WriterPid} ->
+ WriterRef = erlang:monitor(process, WriterPid),
+ NewCounters = orddict:store(WriterPid, {WriterRef, 0}, Counters),
Review comment:
@nickva now that you pointed out the existence of multiple writers for a given `WriterPid` I think this logic to initialize the count to 0 for each writer is wrong. I I think it will cause `maybe_send_data` to use the wrong index when sending chunks to writers of attachments 2..N, because the index is on the set of all chunks for the entire HTTP request. You agree?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org