Skip to content

Make sure to actually wrap the action in 'wrapMonadClientSem' #1786

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

Merged
merged 5 commits into from
Sep 20, 2021

Conversation

isovector
Copy link
Contributor

@isovector isovector commented Sep 17, 2021

Looking through #1781, I noticed we unintentionally changed the logic with respect to how exceptions are handled. We forgot to do any wrapping in wrapMonadClientSem!

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information:
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@isovector isovector requested a review from fisx September 17, 2021 18:30
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I think this is just translating one internal server error into another one, but the idea was to keep the behavior completely indistinguishable, so, good catch!

@isovector
Copy link
Contributor Author

@fisx good to merge, but apparently I don't have the right bits to do it myself.

@fisx
Copy link
Contributor

fisx commented Sep 20, 2021

all tests passed locally.

@fisx fisx merged commit 499c299 into develop Sep 20, 2021
@fisx fisx deleted the wrap-monad-client-sem branch September 20, 2021 08:56
@jschaul
Copy link
Member

jschaul commented Sep 20, 2021

@isovector @fisx please stop merging PRs until CI is green; #1781 introduced a new system-level dependency which resulted in breaking all the CI tests for everyone when it was merged:

docker run -it --entrypoint sh quay.io/wire/spar-schema:latest
 # ldd /usr/bin/spar-schema
	/lib/ld-musl-x86_64.so.1 (0x79110c8fc000)
Error loading shared library libncursesw.so.6: No such file or directory (needed by /usr/bin/spar-schema)
	libxml2.so.2 => /usr/lib/libxml2.so.2 (0x79110c7d3000)
	libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x79110c63a000)
	libz.so.1 => /lib/libz.so.1 (0x79110c620000)
	libssl.so.1.1 => /lib/libssl.so.1.1 (0x79110c59f000)
	libcrypto.so.1.1 => /lib/libcrypto.so.1.1 (0x79110c320000)
	libgmp.so.10 => /usr/lib/libgmp.so.10 (0x79110c2b9000)
	libc.musl-x86_64.so.1 => /lib/ld-musl-x86_64.so.1 (0x79110c8fc000)
	libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x79110c2a5000)
	liblzma.so.5 => /usr/lib/liblzma.so.5 (0x79110c28

all tests passed locally.

That's not entirely correct, ./deploy/dockerephemeral/run.sh fails with the same issue.

@pcapriotti
Copy link
Contributor

That's not entirely correct, ./deploy/dockerephemeral/run.sh fails with the same issue.

To be fair, if you have the library installed locally and you were already running the ephemeral services in the background, you wouldn't notice the issue.

But I agree, we shouldn't merge when CI is red.

@fisx
Copy link
Contributor

fisx commented Sep 20, 2021

Sorry, will look into it now.

I my defense (@isovector had nothing to do with this): I saw and error that I didn't understand, but that did look like something related to concourse or docker trouble, which has happened in the past. When I did a rerun and tried to look at the logs again, concourse wouldn't show them (page wouldn't load). That's when I decided to take the risk.

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