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

Bump ibrowse to 4.4.2 + couchdb patches #3551

Merged
merged 1 commit into from May 10, 2021
Merged

Conversation

nickva
Copy link
Contributor

@nickva nickva commented May 7, 2021

Set the worker_trap_exits = false setting to ensure our replication worker pool properly cleans up worker processes.

Ref: #3208

@wohali wohali mentioned this pull request May 7, 2021
@@ -151,7 +151,7 @@ DepDescs = [
%% Third party deps
{folsom, "folsom", {tag, "CouchDB-0.8.3"}},
{hyper, "hyper", {tag, "CouchDB-2.2.0-6"}},
{ibrowse, "ibrowse", {tag, "CouchDB-4.0.1-2"}},
{ibrowse, "ibrowse", {tag, "CouchDB-4.4.2-1"}},
Copy link
Contributor

Choose a reason for hiding this comment

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

There are dragons there:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea to check. It looks it won't log unless we configure a logger in the app env:

https://github.com/apache/couchdb-ibrowse/blob/CouchDB-4.4.2-1/src/ibrowse_lib.erl#L443-L446

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iilyak would you want to make a PR for upstream to remove that, though with the logging not configured not sure how dangerous it is still?

And we could perhaps also try to upstream the commit to scrub credentials as well. I imagine other users would want to keep credentials not showing up in the logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did more thorough review of ibrowse source code. There are couple of potential issues. 1) debug/trace mode and 2) dumping of message queue using log_msg. I do agree with you that neither of these would become a problem if we wouldn't configure logger.

@nickva nickva force-pushed the bump-ibrowse-to-4.4.2-patches branch from 8d22a48 to 4839fb5 Compare May 7, 2021 21:48
@nickva
Copy link
Contributor Author

nickva commented May 7, 2021

ibrowse has unit tests now, which run and pass. However, they use a deprecated ssl:ssl_accept function.

PR cmullaparthi/ibrowse#172 for upstream

Set the `worker_trap_exits = false` setting to ensure our replication worker
pool properly cleans up worker processes.

Ref: #3208
@nickva nickva force-pushed the bump-ibrowse-to-4.4.2-patches branch from 4839fb5 to 6807dcd Compare May 8, 2021 18:40
@nickva
Copy link
Contributor Author

nickva commented May 10, 2021

Upstream PR was merged, I created a new ibrowse + our patches release and updated the PR. It now passes the CI.

Copy link
Member

@wohali wohali left a comment

Choose a reason for hiding this comment

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

+1 and please backport to 3.x!

@nickva
Copy link
Contributor Author

nickva commented May 10, 2021

@wohali waiting to resolve #3551 (comment)

Copy link
Contributor

@iilyak iilyak left a comment

Choose a reason for hiding this comment

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

+1

@nickva nickva merged commit e69184a into main May 10, 2021
@nickva nickva deleted the bump-ibrowse-to-4.4.2-patches branch May 10, 2021 15:26
jaydoane added a commit that referenced this pull request Jun 18, 2021
With the move from using a forked ibrowse to upstream [1], the
ibrowse options for socks5 proxy settings all changed to a `socks5_`
prefix.

[1] #3551
jaydoane added a commit that referenced this pull request Jun 24, 2021
With the move from using a forked ibrowse to upstream [1], the
ibrowse options for socks5 proxy settings all changed to a `socks5_`
prefix.

[1] #3551
jaydoane added a commit that referenced this pull request Jun 25, 2021
With the move from using a forked ibrowse to upstream [1], the
ibrowse options for socks5 proxy settings all changed to a `socks5_`
prefix.

[1] #3551
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.

None yet

3 participants